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

fix: disabled caching during npm run lint #434

Merged
merged 5 commits into from
Aug 16, 2016

Conversation

shubheksha
Copy link
Contributor

Fixes #339
The .cache directory is no longer created while running npm run lint

@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling b78621c on shubheksha:fix/339 into ba60e9f on mozilla:master.

@kumar303
Copy link
Contributor

This is off to a great start, thanks. I verified that the npm run lint command is working without the cache now 👍

I went back and re-read the issue and remembered the main thing we need is for npm start to use newer:eslint. The npm start command is the one we use in day to day development so it needs to be very fast. It will be executed every time you change a file so it needs to use the eslint cache for efficiency.

The npm start command is an alias for grunt develop which calls the subtask watch:develop. This task is configured to call grunt lint but it should instead call newer:eslint.

As for grunt test, this command should not use the eslint cache (I was mistaken about that). This command runs once and exits so it's not important to use the cache.

@shubheksha
Copy link
Contributor Author

Thanks for the feedback! I will make the specified changes and open a new PR.

@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 14b7b83 on shubheksha:fix/339 into ba60e9f on mozilla:master.

@@ -35,7 +35,7 @@ module.exports = function(grunt) {
grunt.registerTask('test', [
'build-tests',
'mochaTest',
'lint',
'eslint',
Copy link
Contributor

Choose a reason for hiding this comment

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

this can continue to call the lint task, it's the same thing

@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling a8b7c98 on shubheksha:fix/339 into ba60e9f on mozilla:master.

@@ -48,12 +48,9 @@ module.exports = function(grunt) {
grunt.registerTask('lint', 'checks for syntax errors', function() {
if (process.env.SKIP_LINT) {
grunt.log.writeln('lint task skipped because of $SKIP_LINT');
} else if (semver.satisfies(process.version, '< 4.0.0')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

At the top of the file, you can now remove the line that requires the semver module. The lint command in the test suite caught this error for you :) Also, you can now remove semver from package.json because it's not used anywhere else.

@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling c8d2326 on shubheksha:fix/339 into ba60e9f on mozilla:master.

@kumar303
Copy link
Contributor

Thanks for all the fix-ups.

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.

4 participants