Sort dependencies before writing to pom.xml #1113
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1113 +/- ##
==========================================
+ Coverage 65.96% 65.99% +0.03%
==========================================
Files 160 160
Lines 12906 12910 +4
==========================================
+ Hits 8513 8520 +7
+ Misses 3919 3917 -2
+ Partials 474 473 -1 ☔ View full report in Codecov by Sentry. |
| sort.Slice(dm.Dependencies, func(i, j int) bool { | ||
| di := dm.Dependencies[i] | ||
| dj := dm.Dependencies[j] | ||
| if di.GroupID != dj.GroupID { | ||
| return di.GroupID < dj.GroupID | ||
| } | ||
| if di.ArtifactID != dj.ArtifactID { | ||
| return di.ArtifactID < dj.ArtifactID | ||
| } | ||
| if di.Type != dj.Type { | ||
| return di.Type < dj.Type | ||
| } | ||
| if di.Classifier != dj.Classifier { | ||
| return di.Classifier < dj.Classifier | ||
| } | ||
|
|
||
| return di.Version < dj.Version | ||
| return compareDependency(dm.Dependencies[i], dm.Dependencies[j]) |
There was a problem hiding this comment.
Not something to change, but something I've wondered about:
Is there a reason to use sort.Slice over slices.SortFunc, or is it just a preference thing?
I guess technically slices.SortFunc would make this one line instead of two:
slices.SortFunc(dm.Dependencies, compareDependency)
There was a problem hiding this comment.
fwiw the docs technically recommend you do change it:
Note: in many situations, the newer slices.SortFunc function is more ergonomic and runs faster.
though I think when I looked into it, it felt like their comment was more based on what types you were using that lead to different compare functions rather than "we've spent more time optimizing slices.SortFunc than this one" kind of thing?
also kind of related, here's some prior art on how I implemented a chain of compares similar to what you've done with compareDependency - I don't think your function is bad or needs changing, but I figured you might be interested from a consistency POV
Also also, note that there's cmp.Or which might be useful here, but also might not be... 🤷
There was a problem hiding this comment.
Nice to know both slice.SortFunc and cmp.Or!
There was a problem hiding this comment.
sadly cmp.Or only for Go 1.22 :(
There was a problem hiding this comment.
Ek sorry I learned that the same hard way when I first discovered it 😬
| } | ||
|
|
||
| func compareDependency(d1, d2 dependency) int { | ||
| return cmp.Or(cmp.Compare(d1.GroupID, d2.GroupID), cmp.Compare(d1.ArtifactID, d2.ArtifactID), |
There was a problem hiding this comment.
Nit: I think it'd be more readable if you put each condition on a newline
Currently, the write test is a bit flaky because the dependencies to be added are not sorted and their order in pom.xml is not guaranteed.
This PR adds sorting of dependencies before they are going to be written.