publish-announcement: sort versions to be decreasing order #135
Conversation
goversion/goversion.go
Outdated
|
|
||
| // IsNewerThan compares two versions and returns true if the receiver is newer than the argument | ||
| // version. The comparison is done by comparing the major, minor, patch, revision, prerelease and note parts in that | ||
| // order. If all parts are equal, false is returned. It may return an error if a part is not an integer. |
There was a problem hiding this comment.
I tried rewriting this to explain the intent behind "may return an error if a part is not an integer", but I'm ending up not really happy about exposing this method with it implemented this way. I see a few ways forward:
- Preprocessing the versions to actually make sure they're all integers. I think this makes more sense to the user, is more usable, and ends up with a cleaner implementation: https://gist.github.com/dagood/9d36726d640d3138165916636d564ecf
- We could move this back to publish-announcement and unexport it to remove the burden of making it a public API.
- This method could be improved to handle strings as well as numbers to remove
errorand simplify usage and the explanation. Basically: a more generic natural sort, rather than a natural sort if the data happens to meet some constraints.
There was a problem hiding this comment.
@mertakman and I chatted about this and I liked the suggestion to use https://pkg.go.dev/sort#Interface and to simply put non-int-parseable elements at the end of the slice. This is easier to document as part of our exported API and is analogous to how Go sorts (or rather, doesn't sort?) various NaNs.
This is similar to the third alternative above, but we don't really need to make an effort to support "bad" versions in much detail. Also, using sort.Interface to make it more intuitive to use rather than the current func and sort.Slice. Perf isn't a significant factor here given our use cases (not that it's necessarily a problem here).
(In the future we might want to consider restricting what goversion allows, to catch "bad" versions early and eliminate this edge case. From the dev/user's point of view, this would mean catching it in the microsoft-go-infra-release-start pipeline before the version number is used to do anything.)
|
@mertakman I see a force push, but no changes to the sorting code in this PR--just a rebase or something so far? |
cmd/releasego/testdata/publish-announcement/three-branches.golden.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I pushed a few changes, now it LGTM. @mertakman or @gdams, take a look and make sure you agree and feel free to merge. 🙂
No description provided.