make group #3 non-capturing, since we never reference it. #2

Merged
merged 1 commit into from Sep 16, 2016

Conversation

Projects
None yet
5 participants
Contributor

natefinch commented Mar 22, 2016

This makes the code a little easier to follow, and we remove the nested groups which are kind of confusing.

mbruzek commented Mar 22, 2016

I checked the new regular expression with https://regex101.com/ and the following versions came back correctly:

2.0-alpha2
1.  [0-1]   `2`
2.  [2-3]   `0`
3.  [4-9]   `alpha`
4.  [9-10]  `2`
1.26-beta1
1.  [0-1]   `1`
2.  [2-4]   `26`
3.  [5-9]   `beta`
4.  [9-10]  `1`
1.2.3.4
1.  [0-1]   `1`
2.  [2-3]   `2`
4.  [4-5]   `3`
5.  [5-7]   `.4`

Looking at the code, the sub 5 seems OK for this last test case. Are there tests for the version that need to be updated? Is there any other use of the version regex in the code?

Contributor

natefinch commented Mar 22, 2016

both are only ever used in the one spot, and there's a bunch of tests, so I'm pretty confident it's ok.

Member

anastasiamac commented Sep 6, 2016

Please resolve conflicts so that we can review.

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.
Collaborator

bz2 commented Sep 16, 2016

LGTM.

Collaborator

reedobrien commented Sep 16, 2016

👍 All the tests pass.

@reedobrien reedobrien closed this Sep 16, 2016

@natefinch natefinch reopened this Sep 16, 2016

@natefinch natefinch merged commit 94d843f into juju:master Sep 16, 2016

Collaborator

reedobrien commented Sep 16, 2016

sorry, not sure how I hit the close button. must have double tabbed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment