-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cache sections
and numberOfSection
#18
Cache sections
and numberOfSection
#18
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM but is there a test(s) we can include to ensure this works correctly?
// A quick test for if this is the last child is a small optimisation, mainly | ||
// beneficial when the provider has just been appended. | ||
switch children.last { | ||
case .some(.provider(let lastProvider)) where lastProvider === provider: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There are existing tests that run as part of the PR and cover most of the uses but with a couple of extra tests the coverage for the file could probably be ~98%. Adding Codecov to get an idea of coverage change would be useful in the future, in case the existing tests don't cover something added. |
Don't you use Xcode for that while coding? I find it helps me cover the majority early on. |
It's not enabled in the shared scheme but I will commit a change to enable it as part of the PR. Xcode's coverage is useful, but it doesn't cover all cases:
Percentage covered is still not a great metric of code quality, but I have found that having coverage as part of PRs has caught issues earlier. Codecov is free and it's quite easy to setup but I don't have the permission to add it to the organisation. I think it's well worth it though. |
– Just to clarify, I definitely wasn't suggesting Xcode as a replacement. I just meant while coding, I personally find it really helpful to see those per-line numbers, so I know if I've 100% missed a code-path. Especially when there's a few conditions in a specific function for example with complicated paths. On a side note, this approach also sometimes helps identify less testable code as well. |
They have a bash uploader but when running the tests using We can copy the config from https://github.com/JosephDuffy/Persist/blob/master/.github/workflows/tests.yml and it should work 👍 |
@shaps80 I think this should be good to go now? Once merged I'll release 1.1.1. Codecov can be enabled in another PR. |
Yep, looks great thanks! Merge away |
These properties are accessed quite frequently and can be cached with a little extra processing.
This is another finding from screens that have a large number of composed sections.
This and composed-swift/ComposedUI#15 seem to be the main areas of performance issues, although that doesn't mean that once these are working and merged we won't uncover more 😅
Thanks to @bill201207 for their contributions to this change.