-
Notifications
You must be signed in to change notification settings - Fork 59
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
Update Composition.focus to handle Composition.entry #3647
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3647 +/- ##
=========================================
+ Coverage 26.8% 28.0% +1.1%
Complexity 801 801
=========================================
Files 284 283 -1
Lines 15053 14608 -445
Branches 2667 2609 -58
=========================================
+ Hits 4036 4091 +55
+ Misses 10500 9975 -525
- Partials 517 542 +25
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Can you rewrite this by defining a function that takes the values that have the key of either "focus" or "entry", so that you can keep all the code the same other than where you happen to get the values from?
E.g. regardless of whether the key in the composition is "focus" or "entry" all the process that happens to the
{
"reference": ...
"identifier": ...
}
is going to be exactly the same. So the only part that should vary is whether you get that from the a key that's called "focus" or "entry".
Also, not that in the test cases you need to handle at least the cases where
- the composition uses all focus
- the composition uses all entry
- the composition uses both, but not in the same section
- the composition uses both and at least one in the same section (ie verify it always takes either entry or focus when it sees both)
- the composition has neither entry or focus (yes, this is invalid but we need to check it doesn't crash)
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.
Great start!
Closed this in favor of this ##3662 |
IMPORTANT: Where possible all PRs must be linked to a Github issue
Fixes #3617
Engineer Checklist
strings.xml
file./gradlew spotlessApply
and./gradlew spotlessCheck
to check my code follows the project's style guideCode Reviewer Checklist
strings.xml
file