Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upGitHub is where the world builds software
Millions of developers and companies build, ship, and maintain their software on GitHub — the largest and most advanced development platform in the world.
Prevent require-ing bin/shjs #848
Conversation
|
LGTM, but please address comments before landing |
| console.log(); | ||
| process.exit(1); | ||
| } | ||
|
|
||
| if (require.main !== module) { |
nfischer
May 10, 2018
Member
nit: can you move this up? Also, I assume you moved require('../global') down so that it doesn't pollute the test--this is OK, but you need to add a comment explaining why.
nit: can you move this up? Also, I assume you moved require('../global') down so that it doesn't pollute the test--this is OK, but you need to add a comment explaining why.
|
|
||
| if (process.argv.length < 3) { | ||
| console.log('ShellJS: missing argument (script name)'); | ||
| function exit(msg) { |
nfischer
May 10, 2018
Member
nit: rename this function (it's too easy to confuse this with shell.exit()). Suggestion: exitWithErrorMessage()
nit: rename this function (it's too easy to confuse this with shell.exit()). Suggestion: exitWithErrorMessage()
|
@nfischer PTAL |
Codecov Report
@@ Coverage Diff @@
## master #848 +/- ##
======================================
Coverage 95.8% 95.8%
======================================
Files 34 34
Lines 1262 1262
======================================
Hits 1209 1209
Misses 53 53Continue to review full report at Codecov.
|
| exitWithErrorMessage('ShellJS: missing argument (script name)'); | ||
| } | ||
|
|
||
| // we only need ShellJS methods past this point, so require here |
nfischer
May 11, 2018
Member
Our usual style is to import at the top, not to import immediately before use. If there's a real requirement that we import down here, please document that requirement.
I suspect the requirement is: "we must import this after the require.main check, otherwise we pollute the global namespace as a side effect of the disallow require-ing test case."
If that's the real requirement, then you should move this immediately below the require.main check (and indicate this requirement). Otherwise, please explain why this is the appropriate spot, because I can't figure that out.
Our usual style is to import at the top, not to import immediately before use. If there's a real requirement that we import down here, please document that requirement.
I suspect the requirement is: "we must import this after the require.main check, otherwise we pollute the global namespace as a side effect of the disallow require-ing test case."
If that's the real requirement, then you should move this immediately below the require.main check (and indicate this requirement). Otherwise, please explain why this is the appropriate spot, because I can't figure that out.
freitagbr
May 19, 2018
Author
Contributor
Correct, the require at least needs to be after the require.main check, because the error it throws is catchable, and we don't want any side effects, as you said. I also moved it after the process.argv check, because we still don't need any globals at that point, but the fail condition is exiting. There is less of a benefit to require-ing after this check, so I will move it up.
Correct, the require at least needs to be after the require.main check, because the error it throws is catchable, and we don't want any side effects, as you said. I also moved it after the process.argv check, because we still don't need any globals at that point, but the fail condition is exiting. There is less of a benefit to require-ing after this check, so I will move it up.
|
@freitagbr still lgtm |
Fixes #789
This makes it so that
bin/shjscannot berequired as a module, and adds a test for this.