model: Serialize dependency graph edges in some explicit order #8696
Conversation
fviernau
changed the title
fviernau
force-pushed
the
model-dep-graph-edges-serializataion
branch
from
f54967c
to
23d9873
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8696 +/- ##
============================================
- Coverage 67.88% 67.86% -0.02%
Complexity 1165 1165
============================================
Files 244 244
Lines 7732 7734 +2
Branches 865 865
============================================
Hits 5249 5249
- Misses 2124 2126 +2
Partials 359 359
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fviernau
force-pushed
the
model-dep-graph-edges-serializataion
branch
from
23d9873
to
6422045
Compare
| @@ -102,7 +102,7 @@ data class DependencyGraph( | |||
| * A list with the edges of this dependency graph. By traversing the edges, the dependencies of packages can be | |||
| * determined. | |||
| */ | |||
| val edges: List<DependencyGraphEdge>? = null | |||
| val edges: Set<DependencyGraphEdge>? = null | |||
There was a problem hiding this comment.
The comment in line 102 should now also say "set" instead of "list". Also, the commit message should probably also mention that order is not important either (even if a set usually maintains insertion order).
| @@ -179,7 +179,7 @@ class DependencyGraphBuilder<D>( | |||
| ) | |||
| } | |||
|
|
|||
| private fun Collection<DependencyGraphEdge>.removeCycles(): List<DependencyGraphEdge> { | |||
| private fun Set<DependencyGraphEdge>.removeCycles(): Set<DependencyGraphEdge> { | |||
There was a problem hiding this comment.
In the spirit of "be lenient when consuming, be strict when producing", should the receiver stay a Collection?
There was a problem hiding this comment.
I believe in general it makes sense, in this case I decided against it so that one does not have to even think about what happens when input contains duplicates, because the API is private (small scope), and because there is no use besides the Sets one.
fviernau
force-pushed
the
model-dep-graph-edges-serializataion
branch
from
6422045
to
8787c8a
Compare
It is not necessary to allow for having multiple edges between any pair of nodes. So, always operate with sets to reflect that. Note that order of edges is not important either (even if a set usually maintains insertion order). Signed-off-by: Frank Viernau <[email protected]>
Improve human readability and make the serialized ordering independent from the insertion order. Signed-off-by: Frank Viernau <[email protected]>
Use the `RewriteTestAssetsCommand` to re-align the test assets with the new ordered serialization of `edges`. Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
model-dep-graph-edges-serializataion
branch
from
8787c8a
to
e872dcf
Compare
Make serialized order independent of insertion order in order to remove the need to update the tests when changing insertion order in upcoming changes.
Part of #6235.