Skip to content
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

Handle a version with a dash in the prerelease tag #60

Conversation

carolynvs
Copy link

@carolynvs carolynvs commented Jul 26, 2017

When parsing a constraint, before converting dashes to a range, check if it is a valid version first. That way a version with multiple dashes, like v1.0.0-12-abc123 isn't turned into a range of 1.0.0 to 12-abc123.

Fixes #59.

When parsing a constraint, before converting dashes to a range,
check if it is a valid version first. That way a version with multiple
dashes, like v1.0.0-12-abc123 isn't turned into a range of
1.0.0 to 12-abc123.
@carolynvs carolynvs force-pushed the parse-constraints-with-dash-in-pre branch from af60833 to a93e51b Compare July 26, 2017 23:05
@carolynvs
Copy link
Author

My initial attempt checked if the version was valid using only the regex, but that doesn't work well when legitimate ranges look exactly like versions, such as 2-3. I'm hoping this is an acceptable compromise, where if the segment starts with a v, assume that parsing as a version is preferred, instead of a range.

@@ -13,6 +13,9 @@ func rewriteRange(i string) string {
}
o := i
for _, v := range m {
if strings.HasPrefix(v[0], "v") && versionRegex.MatchString(v[0]) {
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the method of detecting a leading v. The leading v is not part of the SemVer spec. Applications other than dep import this package and don't require the v the way the go community wants to.

Any ideas on another method? The 1.x version looks for a space on both sides of the - for a range.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for letting this drop off my radar entirely!

Would you be up for requiring that ranges must have a space around the dash? Right now the v2 branch has tests that support 2-3 which made it nearly impossible for me to do what I needed without the leading v trick. If we went back to requiring spaces for a range, then it wouldn't be a problem.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@carolynvs sorry this dropped of my radar. If there was a space surrounding the dash it would work for me. The currently implementation on the v1 branch requires one or more space characters on each side of the dash.

This would be so much easier if there was a spec for ranges.

@carolynvs
Copy link
Author

Closing in favor of #67

@carolynvs carolynvs closed this Apr 2, 2018
@carolynvs carolynvs deleted the parse-constraints-with-dash-in-pre branch April 2, 2018 19:15
@tamalsaha
Copy link

@carolynvs, dep's Gopkg file is broken since your branch is gone. https://github.com/golang/dep/blob/master/Gopkg.toml#L1 . Can you please look into this?

@carolynvs carolynvs restored the parse-constraints-with-dash-in-pre branch April 3, 2018 02:31
@carolynvs
Copy link
Author

@tamalsaha I've repushed the branch, sorry about that. Can you help me understand what exactly broke immediately once the branch was gone? e.g. building dep, or something else?

@tamalsaha
Copy link

Thanks @carolynvs ! We use the gps library in https://github.com/kubepack/pack . So, I tried to revendor and it broke.

@carolynvs
Copy link
Author

carolynvs commented Apr 3, 2018

Doh! Sorry about that. GitHub gave me a big button to press when I closed the PR and ... I have no self control. 😂 I promise to keep that branch around forever and ever now.

@carolynvs
Copy link
Author

Working on getting dep in sync with the 2.x branch here: golang/dep#1792

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants