Skip to content

Fixes #1070 #1071

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

Closed
wants to merge 1 commit into from
Closed

Fixes #1070 #1071

wants to merge 1 commit into from

Conversation

fsmaia
Copy link

@fsmaia fsmaia commented Dec 13, 2016

Fixes issue #1070 by adding support to npm config for mirror variables.

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

I think a better approach is to add them as (undocumented) command line flags, e.g. --iojs_org_mirror. node-gyp will automatically pick up the npm_config_-prefixed environment variable.

Also, can you add tests, keep lines <= 80 columns and write up a nice commit log?

@fsmaia
Copy link
Author

fsmaia commented Dec 19, 2016

By adding them as command line flags you mean relying on a dependency like command-line-args? And then reading the config either by environment variable and command-line (which include npm_config automatic inheritance)? I never worked in this config-to-cli inheritance before, but I can try with this library. Any other suggestion?

About the tests and nice commit log, I can work on that (linking to issue is just a personal preference).

About keeping lines <= 80 columns, did you consider writing an .editorconfig file to project?

Thanks in advance!

@bnoordhuis
Copy link
Member

By adding them as command line flags you mean relying on a dependency like command-line-args?

We use nopt and some homegrown logic. You can find the list of switches in lib/node-gyp.js, grep for configDefs and parseArgv.

@bnoordhuis
Copy link
Member

About keeping lines <= 80 columns, did you consider writing an .editorconfig file to project?

There are existing style violations and I don't think it's worth the churn to fix them, I just try to keep new ones from sneaking in. :-)

@fsmaia
Copy link
Author

fsmaia commented Dec 20, 2016

We use nopt and some homegrown logic. You can find the list of switches in lib/node-gyp.js, grep for configDefs and parseArgv.

When switching to parseArgv instead of environment variable, should I refactor this entry too? By the way switching to parseArgv involves moving it from node-gyp and exposing it in a higher instance (to be used in node-gyp and install). Are you sure this is the moment to do this refactoring?

There are existing style violations and I don't think it's worth the churn to fix them, I just try to keep new ones from sneaking in. :-)

In order to not to reproduce these violations again, could you give me the specifications?

@fsmaia
Copy link
Author

fsmaia commented Dec 20, 2016

Reviewing the code a little further I think parseArgv isn't a nice approach. It is useful to parse CLI arguments for node-gyp, but in this case we are dealing with environment configuration used in installation step, and at this time the CLI isn't even available.

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