Changes to qualified scopes for dependency graphs #9844
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9844 +/- ##
=========================================
Coverage 68.12% 68.12%
+ Complexity 1293 1292 -1
=========================================
Files 250 250
Lines 8843 8840 -3
Branches 918 917 -1
=========================================
- Hits 6024 6022 -2
+ Misses 2432 2431 -1
Partials 387 387
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
sschuberth
changed the title
sschuberth
force-pushed
the
dependency-graph-simplifications
branch
2 times, most recently
from
8b5f983 to
8b4002e
Compare
| @@ -157,28 +157,24 @@ data class DependencyGraph( | |||
| * Transform the data stored in this object to the classical layout of dependency information, which is a set of | |||
| * [Scope]s referencing the packages they depend on. | |||
| */ | |||
| fun createScopes(): Set<Scope> = createScopesFor(scopes, unqualify = true) | |||
| fun createScopes(): Set<Scope> = createScopesFor(scopes) | |||
There was a problem hiding this comment.
commit-msg: how scope -> how scopes
| @@ -95,11 +95,11 @@ class AnalyzerResultBuilderTest : WordSpec() { | |||
| private val depRef3 = DependencyReference(0, issues = listOf(issue5)) | |||
|
|
|||
| private val scopeMapping1 = mapOf( | |||
| DependencyGraph.qualifyScope(project1, "scope-1") to listOf(RootDependencyIndex(0)), | |||
| DependencyGraph.qualifyScope(project3, "scope-2") to listOf(RootDependencyIndex(1)) | |||
| DependencyGraph.qualifyScope(project1.id, "scope-1") to listOf(RootDependencyIndex(0)), | |||
There was a problem hiding this comment.
Hm, the previous code was a bit less repetitive.
I also do not understand why the symmetry becomes clearer and why this is favorable / what value that brings.
There was a problem hiding this comment.
Hm, the previous code was a bit less repetitive.
Do you mean "verbose" instead of "repetitive"? Because the refactoring does not introduce any code repetition, as I see it.
What I like about only taking the ID in qualifyScope() is that it makes more clear which properties of a Project play a role in qualification, i.e. only the Identifier. Also that overload doe snot really save much typing, as adding .id is trivial.
I also do not understand why the symmetry becomes clearer and why this is favorable / what value that brings.
I was planning to change unqualifyScope() to also return the encoded project ID, of which you can reconstruct an Identifier, but not a whole Project. That's why I believe qualifyScope() should not take a Project if unqualifyScope() cannot reconstruct it.
All callers set `unqualify` to `true` (except for one usage in a test), so hard-code that to simplify code. Qualified scope names should be an implementation detail for how scopes are disambiguated across projects. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
Inline the function in favor of only keeping the function that takes a project's ID. This makes the symmetry to `unqualifyScope()` clearer. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>
sschuberth
force-pushed
the
dependency-graph-simplifications
branch
from
8b4002e to
fdaa917
Compare
Please have a look at the individual commit messages for the details.