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

gyp: vendor in GYP3 6.0.3 #1845

Closed
wants to merge 4 commits into from
Closed

gyp: vendor in GYP3 6.0.3 #1845

wants to merge 4 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Jul 24, 2019

refack/GYP3@8806c63

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

@@ -421,7 +423,7 @@ def FixVCMacroSlashes(s):


def ConvertVCMacrosToMSBuild(s):
"""Convert the MSVS macros found in the string to the MSBuild equivalent.
"""Convert the the MSVS macros found in the string to the MSBuild equivalent.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the the?

@@ -476,8 +479,8 @@ def ConvertToMSBuildSettings(msvs_settings, stderr=sys.stderr):
(msvs_tool_name, msvs_setting)),
stderr)
else:
print('Warning: unrecognized tool %s while converting to '
'MSBuild.' % msvs_tool_name, file=stderr)
print(('Warning: unrecognized tool %s while converting to '
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra parens not needed.

'\n' \
' export *\n' \
' module * { export * }\n' \
'}\n' % (binary, binary)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use () around the whole string and lose the backslashes.

Returns True if |string| is a canonical integer form str.
The canonical form is such that str(int(string)) == string.
"""
if type(string) is not str:
Copy link
Contributor

@cclauss cclauss Jul 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isinstance()?

from functools import reduce

if 'reduce' not in __builtins__:
from functools import reduce
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change is not need. from functools import reduce works everywhere.

@rvagg
Copy link
Member

rvagg commented Jul 25, 2019

Heyhey! Welcome back @refack, I hope. Can you give us a status update? I'd love to have you maintain refack/GYP and vendor it in here but it'd be nice to know whether you're going to be around to maintain it. Have you just been taking time off?

.idea/deployment.xml Outdated Show resolved Hide resolved
@@ -1,4 +1,5 @@
Copyright (c) 2009 Google Inc. All rights reserved.
Copyright (c) 2019 Refael Ackermann<[email protected]>. All rights reserved.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might need to get legal confirmation that a stacked copyright is possible, my understanding is that it's locked in to the original claimant and you just have to live with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stacked copyright is the legally recommended way to claim rights of forked work. In this case, Google maintains the rights to work done till the fork.

@refack
Copy link
Contributor Author

refack commented Jul 26, 2019

Have you just been taking time off?

Yep. Taking a bit of time off.

Can you give us a status update? I'd love to have you maintain refack/GYP and vendor it in here but it'd be nice to know whether you're going to be around to maintain it.

I do hope to keep maintaining GYP3 for foreseeable future (Independent of my contributions to node related stuff).

@cclauss
Copy link
Contributor

cclauss commented Jul 26, 2019

A few conflicting files...

@mcollina
Copy link
Member

I think the README should be updated to reflect that we are now bundling GYP3, and not gyp.

@cclauss
Copy link
Contributor

cclauss commented Sep 12, 2019

Can we please rebase this PR?

@cclauss
Copy link
Contributor

cclauss commented Oct 24, 2019

Closing. The requested rebase above did not happen and I sense we are beyond the point of no return on this one.

@cclauss cclauss closed this Oct 24, 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.

4 participants