/version

fix parse for tags w/ double digit patch numbers #3

Merged
merged 1 commit into from Jun 3, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@natefinch @bz2
@natefinch
Contributor

natefinch commented Jun 3, 2016 •
Edited 1 time

Turns out the tag group (\w+) would consume all but the last digit of the patch, because \w+ matches numbers as well as letters. In talking to Martin, we're pretty sure we don't need to match anything but a-z for the tag name, so I changed it to that, and fixed the tests to match this assumption.
version_test.go
@@ -45,14 +45,13 @@ func (*suite) TestCompare(c *gc.C) {
{"2.0.0.0", "2.0.0.0", 0},
{"2.0.0.1", "2.0.0.0", 1},
{"2.0.1.10", "2.0.0.0", 1},
- {"2.0-_0", "2.0-00", 1},
@natefinch

natefinch Jun 3, 2016

Contributor

this was removed because 2.0-00 is not actually a valid version, so it errors out (it has a zero length tag specified). It used to work because the tag was getting parsed as "0"

version_test.go
@@ -106,6 +105,9 @@ var parseTests = []struct {
v: "1.21-alpha1.1",
expect: version.Number{Major: 1, Minor: 21, Patch: 1, Tag: "alpha", Build: 1},
+}, {
+ v: "1.21-alpha10",
@natefinch

natefinch Jun 3, 2016

Contributor

this is the new failing test that I added before fixing the bug.

@bz2
Collaborator

bz2 commented Jun 3, 2016

LGTM. Lets make the tag match just [a-z]+ as that's all we've used so far.
@natefinch
fix the regex so the tag group won't match any of the patch numbers 
We don't actually want anything but a-z in the tag name
463fc34

@natefinch natefinch merged commit 4ae6172 into juju:master Jun 3, 2016

babbageclunk added a commit to babbageclunk/version that referenced this pull request Sep 15, 2016

@babbageclunk
Update test to match changed juju/version behaviour 
Since commit 4ae6172 (juju#3),
version.Number.Tag will never include any digits, so this test was
incorrect.
340c221

@babbageclunk babbageclunk referenced this pull request in juju/charm Sep 15, 2016

Merged

Update test to match changed juju/version behaviour #219

jujubot added a commit to juju/charm that referenced this pull request Sep 15, 2016

@jujubot
Merge pull request #219 from babbageclunk/fix-test 
Update test to match changed juju/version behaviour

Since commit [4ae6172](juju/version@4ae6172) (juju/version#3), `version.Number.Tag` will never include any digits, so this test became incorrect.
2f2476d

natefinch added a commit that referenced this pull request Sep 16, 2016

@natefinch
make group #3 non-capturing, since we never reference it. 
This makes the code a little easier to follow, and we remove the nested groups which are kind of confusing.
This commit was signed with a verified signature.
@natefinch natefinch Nate Finch
GPG key ID: F5F09E8A6BAA1CCA Learn about signing commits
91c3365

natefinch added a commit that referenced this pull request Sep 16, 2016

@natefinch
make group #3 non-capturing, since we never reference it. (#2) 
This makes the code a little easier to follow, and we remove the nested groups which are kind of confusing.
94d843f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment