Skip to content

allow double-digit versions in XcodeVersion#1854

Closed
roryqueue wants to merge 1 commit into
nodejs:masterfrom
roryqueue:master
Closed

allow double-digit versions in XcodeVersion#1854
roryqueue wants to merge 1 commit into
nodejs:masterfrom
roryqueue:master

Conversation

@roryqueue

@roryqueue roryqueue commented Aug 13, 2019

Copy link
Copy Markdown

Fixes #1849

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

@cclauss

cclauss commented Aug 13, 2019

Copy link
Copy Markdown
Contributor

Related to #1849 ?

@roryqueue

Copy link
Copy Markdown
Author

yes, relates-- sorry at work and not able to dig further atm, but that change seems to fix

@roryqueue

Copy link
Copy Markdown
Author

(or rather, obviously doesn't work given it's failing, but the regex change matches both 9.X and 10.X, where as the other one fails on 10.X, so could be a hint to a real fix)

@rvagg

rvagg commented Sep 25, 2019

Copy link
Copy Markdown
Member

Sorry, I'm not a python person but this looks invalid to me, \d+ will match double digits just as well as \d?\d in any normal regex engine. Can I close this or is someone willing to dispute that Python does something funky with regexes that makes this necessary?

@cclauss cclauss added the Python label Sep 25, 2019
@cclauss

cclauss commented Sep 25, 2019

Copy link
Copy Markdown
Contributor

Can we just replace this line with version = ".".join(version.split(".")[:3]) and move on? Not a fan of regex for trivial use cases.

@cclauss

cclauss commented Sep 30, 2019

Copy link
Copy Markdown
Contributor

I believe that #1890 fixed this. Please rebase and reverify.

@cclauss

cclauss commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

Conflicts to resolve.

@rvagg

rvagg commented Oct 2, 2019

Copy link
Copy Markdown
Member

I think this is dealt with already and the solution posed here isn't correct as far as I can tell

@rvagg rvagg closed this Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Could not get CLTVersion

3 participants