-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(gyp): update gyp to v0.9.6 #2481
Conversation
couple of windows issues there but rubber stamp from me if you can figure those out |
gyp/pylib/gyp/msvs_emulation.py
Outdated
@@ -67,7 +74,8 @@ def EncodeRspFileList(args): | |||
program = call + " " + os.path.normpath(program) | |||
else: | |||
program = os.path.normpath(args[0]) | |||
return program + " " + " ".join(QuoteForRspFile(arg) for arg in args[1:]) | |||
return (program + " " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks different lint rules result in this. Should we skip gyp lib in this repo or unify the rule ? @cclauss
Run flake8 . --ignore=E203,W503 --max-complexity=101 --max-line-length=88 --show-source --statistics
./gyp/pylib/gyp/msvs_emulation.py:77:27: W504 line break after binary operator
return (program + " " +
^
1 W504 line break after binary operator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put the + at the beginning of the next line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add this rule in https://github.com/nodejs/gyp-next ?
Also should we disable flake8 test for this repo since gyp sources is an upstream project ?
@rvagg This id ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rubber stamp from me
changes to path handling on windows always makes me nervous since we get so many reports related to paths but 🤞 I trust you folks know best!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Checklist
npm install && npm test
passesDescription of change