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

Improve install script #663

Merged
merged 2 commits into from
Feb 12, 2015
Merged

Improve install script #663

merged 2 commits into from
Feb 12, 2015

Conversation

rubennorte
Copy link
Contributor

The main goal of this PR is to solve an error. When there are some kind of errors downloading the binary file (network errors mostly) the file where it was going to be downloaded to is not unlinked and left as an empty file and then the build script "thinks" that the download went ok and does not do the manual build. I changed the code to create the file just when it is necessary.

var proxyUrl;

if (!er) {
['https-proxy', 'proxy', 'http-proxy'].some(function(setting) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW this could be reduced to

var proxyUrl = ['https-proxy', 'proxy', 'http-proxy'].filter(function(setting) { 
  return conf.get(setting); 
})[0];

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even simpler yet?

var env = process.env;
options.proxy = conf.get('https-proxy') || conf.get('proxy') || conf.get('http-proxy') || 
                env.HTTPS_PROXY || env.https_proxy || env.HTTP_PROXY || env.http_proxy;

@am11
Copy link
Contributor

am11 commented Feb 11, 2015

Thanks for your contribution. 👍

My hands are full with this binary patch thingy to confirm the changes. @xzyfer, can you please take a look if this safe to merge at this point?

@xzyfer
Copy link
Contributor

xzyfer commented Feb 11, 2015

LGTM. Waiting on CI.

am11 added a commit that referenced this pull request Feb 12, 2015
@am11 am11 merged commit 0e3d809 into sass:master Feb 12, 2015
@am11
Copy link
Contributor

am11 commented Feb 12, 2015

🎉

@rubennorte rubennorte deleted the improve-install-script branch February 13, 2015 19:11
Friendly-users referenced this pull request in Friendly-users/node-sass Jul 9, 2024
-----
It is inappropriate to include political and offensive content in public code repositories.

Public code repositories should be neutral spaces for collaboration and community, free from personal or political views that could alienate or discriminate against others. Political content, especially that which targets or disparages minority groups, can be harmful and divisive. It can make people feel unwelcome and unsafe, and it can create a hostile work environment.

Please refrain from adding such content to public code repositories.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants