Read jshint version from package.json #5908
Conversation
| @@ -1459,7 +1459,15 @@ target.lint = function() { | |||
| var jshintPath = path.normalize('./node_modules/.bin/jshint'); | |||
| if (!test('-f', jshintPath)) { | |||
| echo('jshint is not installed -- installing...'); | |||
| exec('npm install [email protected]'); // TODO read version from package.json | |||
| // Read the jshint version to be installed from package.json. | |||
| fs.readFile('package.json', 'utf8', function (error, data) { | |||
There was a problem hiding this comment.
Can you please read the file synchronously instead, i.e. change fs.readFile to fs.readFileSync, since the code below relies on jshint being immediately available?
Otherwise, when jshint isn't installed, linting will fail and require you to do node make lint twice (once to install, and once to actually lint).
Obviously, you could make the rest of the code async to handle this. But given that jshint should only need to be installed once, I don't think we should complicate the rest of the function unnecessarily.
There was a problem hiding this comment.
Done. I think this also makes the code a bit cleaner and indeed it fixes the problem that one would have to re-run node make lint when it was not yet installed. I have tested this with jshint installed, with jshint uninstalled and with jshint uninstalled and no package.json and all possibilities yield an expected result now. Excellent, thank you!
timvandermeij
force-pushed
the
read-package-json
branch
from
3f91d08 to
35d5a80
Compare
| var configuration = JSON.parse(rawConfiguration); | ||
| exec('npm install jshint@' + configuration.devDependencies.jshint); | ||
| } catch(e) { | ||
| echo('package.json does not exist -- aborting...'); |
There was a problem hiding this comment.
Nit: You might want to return or throw after this line, so that the code actually aborts, otherwise the print statement is slightly wrong :-)
Super-nit: How about adding a space between catch and (e) { above? :-P
There was a problem hiding this comment.
Good catch! Not sure how I managed to miss that... Both issues have been fixed :-)
timvandermeij
force-pushed
the
read-package-json
branch
from
35d5a80 to
2529990
Compare
|
/botio lint |
From: Bot.io (Linux)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/704e857ebf64d03/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_lint from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/b37e94837a2ffb6/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/704e857ebf64d03/output.txt Total script time: 0.84 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/b37e94837a2ffb6/output.txt Total script time: 0.95 mins
|
Read jshint version from package.json
|
Really good, thanks for doing this! |
The previous commit implements a check for ShellJS and otherwise prompts the user to run "npm install". New clones of the codebase will need "npm install" for ShellJS and therefore automatically install jshint. Existing clones of the codebase will also need "npm install" again since ShellJS needs to be installed using NPM as it is not in the "external" folder anymore. Since everyone will get this prompt and install everything automatically, we will never reach this code path anymore. This patch makes mozilla#5908 obsolete and reduces code complexity for the lint target. Thanks to @Snuffleupagus for noticing this!
Resolves another TODO in the codebase.