Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Introduce Masterminds/semver #374
Introduce Masterminds/semver #374
Changes from 2 commits
e3bb5d5
ac07fe1
f005767
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
worth including the blang '!=' equivalent here? (e.g. "!1.2.3")
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.
Probably.
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.
Nit: the asymmetry of
VersionBlang()
stuff being parsed duringb.loadPackage()
and the masterminds version being parsed here every time is maybe a bit unexpected and feels a bit off?Other potential options:
b.semVersion
, parse both duringloadPackage()
loadPackage
) using the blang version fields (this might be nice because its one fewer error to handle)Version()
method just return a string and expect that caller to do the parsing.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.
Another option: do the (option 2) conversion from blang version to masterminds version in the
InMastermindsVersionRange()
function.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.
TL;DR: this is what happens when you try to optimize code coverage!
Masterminds.Version
struct has no user-accessible fields, so you can't create the structure that way. It actually makes more sense to do ablang.Version.String()
and feed that intoMasterminds.NewVersion()
, but it's the same error handling problem as option 1.EDIT: Using just one semver library would make this moot.
EDIT 2: Option 3 is probably the cleanest in terms of API cleanliness, but not efficiency.
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.
AFAICT, this means passing an blang structure into a mastermind-named function, which seems bad form.
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 don't think so.
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.
Sorry, I was thinking of this function: https://github.com/Masterminds/semver/blob/2f39fdc11c33c38e8b8b15b1f04334ba84e751f2/version.go#L214
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.
Although option 3 might be the cleanest, it is changing the behavior of
sortVersion()
.Anything I do to try to fix the nit will negatively impact code coverage.
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.
IMO, trying to cover every error path just leads to brittle tests that make refactoring hard. I think as long as we're covering the happy paths, and the coverage "regression" is only happening because of some refactoring of when/how errors are handled, we shouldn't spend time dealing in the 1s or .1s of coverage percentages.
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.
Is there a way to specify a (minimum, err maximum? negative) threshold for CodeCov?
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 of the pre and metadata fields, etc. it's just easier to do all conversions via
.String()
.