-
Notifications
You must be signed in to change notification settings - Fork 313
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
model(refactor)!: Simplify the DependencyNavigator
API
#9482
base: main
Are you sure you want to change the base?
Conversation
64b5f40
to
1993e31
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9482 +/- ##
=========================================
Coverage 67.94% 67.94%
+ Complexity 1292 1290 -2
=========================================
Files 249 249
Lines 8798 8798
Branches 912 912
=========================================
Hits 5978 5978
Misses 2434 2434
Partials 386 386
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
Identifier("Maven:org.scalactic:scalactic_2.12:3.0.4") | ||
) | ||
} | ||
scopeDependencies should containAll( |
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.
Why not containExactlyInAnyOrder
?
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.
Because not all IDs are expected to be contained. This matches the original code which was also using containAll
per scope for a few exemplary IDs, plus haveSize
assertions for the real number of IDs.
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.
I assumed this should be all packages because you dropped the haveSize
check. If not, I think you should reintroduce it to match the test name.
projectDependencies should containExactlyInAnyOrder(expectedDependencies) | ||
} | ||
|
||
"support filtering the dependencies of a project" { |
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.
Isn't this the same as "return dependencies from all scopes that match a filter criteria"?
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.
Almost, except that custom filtering is combined with maxDepth
here. This is a redundancy that was present originally. But I'm fine with dropping either test.
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.
Then maybe rename this test to describe that it's testing the combination of a filter and maxDepth
.
1993e31
to
532244e
Compare
Reduce the number of very similar overloads and simplify return types in favor of calling fewer "orthogonal" functions in combination, if required. This also aligns the naming of functions to be more consistent. Signed-off-by: Sebastian Schuberth <[email protected]>
532244e
to
8eb6337
Compare
Reduce the number of very similar overloads and simplify return types in favor of calling fewer "orthogonal" functions in combination, if required. This also aligns the naming of functions to be more consistent.