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

allow double-digit versions in XcodeVersion #1854

Closed
wants to merge 1 commit into from

Conversation

roryqueue
Copy link

@roryqueue roryqueue commented Aug 13, 2019

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
Copy link
Contributor

cclauss commented Aug 13, 2019

Related to #1849 ?

@roryqueue
Copy link
Author

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

@roryqueue
Copy link
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
Copy link
Member

rvagg commented Sep 25, 2019

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
Copy link
Contributor

cclauss commented Sep 25, 2019

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
Copy link
Contributor

cclauss commented Sep 30, 2019

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

@cclauss
Copy link
Contributor

cclauss commented Oct 2, 2019

Conflicts to resolve.

@rvagg
Copy link
Member

rvagg commented Oct 2, 2019

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