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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion constraints_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ func TestParseConstraint(t *testing.T) {
includeMin: true,
includeMax: false,
}, false},
{"^1.1.0-12-abc123", rangeConstraint{
min: Version{major: 1, minor: 1, patch: 0, pre: "12-abc123"},
max: newV(2, 0, 0),
includeMin: true,
includeMax: false,
}, false},
}

for _, tc := range tests {
Expand All @@ -70,7 +76,7 @@ func TestParseConstraint(t *testing.T) {
}

if !constraintEq(tc.c, c) {
t.Errorf("Incorrect version found on %s", tc.in)
t.Errorf("%q produced constraint %q, but expected %q", tc.in, c, tc.c)
}
}
}
Expand Down Expand Up @@ -331,6 +337,12 @@ func TestNewConstraintIC(t *testing.T) {
max: newV(2, 0, 0),
includeMin: true,
}, false},
{"v1.1.0-12-abc123", rangeConstraint{
min: Version{major: 1, minor: 1, patch: 0, pre: "12-abc123"},
max: newV(2, 0, 0),
includeMin: true,
includeMax: false,
}, false},
}

for _, tc := range tests {
Expand Down Expand Up @@ -548,6 +560,7 @@ func TestRewriteRange(t *testing.T) {
{"2-3", ">= 2, <= 3"},
{"2-3, 2-3", ">= 2, <= 3,>= 2, <= 3"},
{"2-3, 4.0.0-5.1", ">= 2, <= 3,>= 4.0.0, <= 5.1"},
{"v2-3, 2-3", "v2-3,>= 2, <= 3"},
}

for _, tc := range tests {
Expand Down
3 changes: 3 additions & 0 deletions parse.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

}
t := fmt.Sprintf(">= %s, <= %s", v[1], v[11])
o = strings.Replace(o, v[0], t, 1)
}
Expand Down