Skip to content

install: Fixing NameError - #1628

Closed
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:patch-1
Closed

install: Fixing NameError#1628
thefourtheye wants to merge 1 commit into
nodejs:masterfrom
thefourtheye:patch-1

Conversation

@thefourtheye

Copy link
Copy Markdown
Contributor

If len(args) is lesser than two, then install_path = dst_dir + node_prefix + '/' would throw a NameError, because dst_dir will not be defined yet. So, we are assigning an empty string as the default value, if len(args) is lesser than or equal to two.

@Fishrock123Fishrock123 added the build Issues and PRs related to build files or the CI. label May 5, 2015
@mscdex

Copy link
Copy Markdown
Contributor

The commit message should have a subsystem prefix like tools: or maybe build:

@bnoordhuis

Copy link
Copy Markdown
Member

What @mscdex said, see e.g. 72f1b34 for an example. Apart from that LGTM.

@thefourtheye
thefourtheyeforce-pushed the patch-1 branch 2 times, most recently from f151947 to 850610cCompareMay 5, 2015 17:49
If `len(args)` is lesser than two, then
`install_path = dst_dir + node_prefix + '/'` would throw a `NameError`,
because `dst_dir` will not be defined yet. So we are assigning `''` as
the default value.
@thefourtheye

Copy link
Copy Markdown
ContributorAuthor

@mscdex@bnoordhuis Thanks Guys. I force pushed my commit. Please check now.

@thefourtheyethefourtheye changed the title Fixing NameErrorinstall: Fixing NameErrorMay 6, 2015
Fishrock123 pushed a commit that referenced this pull request May 7, 2015
If `len(args)` is less than two, then
`install_path = dst_dir + node_prefix + '/'` would throw a `NameError`,
because `dst_dir` will not be defined yet. So we are assigning `''` as
the default value.
PR-URL: #1628
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@Fishrock123

Copy link
Copy Markdown
Contributor

Thanks, landed in b97b96d

@thefourtheye
thefourtheye deleted the patch-1 branch May 7, 2015 16:45
@rvaggrvagg mentioned this pull request May 7, 2015
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request May 19, 2015
If `len(args)` is less than two, then
`install_path = dst_dir + node_prefix + '/'` would throw a `NameError`,
because `dst_dir` will not be defined yet. So we are assigning `''` as
the default value.
PR-URL: nodejs#1628
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

buildIssues and PRs related to build files or the CI.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thefourtheye@mscdex@bnoordhuis@Fishrock123