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

Set ESLint rules as errors while preventing hooks/CI from failing #6945

Merged
merged 1 commit into from
Mar 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .eslines.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Are we ever reading this file?

Copy link
Author

Choose a reason for hiding this comment

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

Oh, it had a typo, sorry. This was fixed (and updated with new config) within the latest changes.

Copy link
Member

Choose a reason for hiding this comment

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

👍

"rulesNotToDowngrade": ["no-unused-vars"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this option? I tried referencing the project documentation but it seems either unnecessary in that we should leverage errors as defined in our configuration or invalid in that it's assuming that all errors are warnings if not modified in the current changeset.

Copy link
Contributor

Choose a reason for hiding this comment

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

The core idea of eslines is that it downgrades errors to warnings if they happen in lines not modified in the current branch. This array contains the rules that won't be downgraded no matter whether the line was modified or not. I tried to convey that idea in the config section of the documentation.

Different teams can have different use cases for this. Ours, and one that I'd recommend for every project, is to not downgrade no-unused-vars, as this is a case where modifying a line would cause a lint error on a different line - imports that become unused, for example. I could introduce this recommendation in the eslines docs.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've talked privately to aduth about this and all is clear now.

"remote": "origin/master",
"processors": {
"default": "lines-modified",
"master": "parsing-errors"
}
}
69 changes: 2 additions & 67 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,73 +10,8 @@ module.exports = {
globals: {
asyncRequire: true
},
// Ideally, we should not have a `rules` block here at all, save for some
// Calypso-specific rules (no-unused-expressions, camelcase). The remainder
// are rules we cannot yet flag as errors, and should be removed over time
// as outstanding issues are resolved.
rules: {
'array-bracket-spacing': [ 1, 'always' ],
'brace-style': [ 1, '1tbs' ],
// REST API objects include underscores
camelcase: 0,
'comma-spacing': 1,
curly: 1,
'computed-property-spacing': [ 1, 'always' ],
'func-call-spacing': 1,
indent: [ 1, 'tab', { SwitchCase: 1 } ],
'jsx-quotes': [ 1, 'prefer-double' ],
'key-spacing': 1,
'keyword-spacing': 1,
'max-len': [ 1, { code: 140 } ],
'new-cap': [ 1, { capIsNew: false, newIsCap: true } ],
'no-else-return': 1,
'no-extra-semi': 1,
'no-multiple-empty-lines': [ 1, { max: 1 } ],
'no-multi-spaces': 1,
'no-restricted-imports': [ 1, 'lib/sites-list', 'lib/mixins/data-observe' ],
'no-restricted-modules': [ 1, 'lib/sites-list', 'lib/mixins/data-observe' ],
'no-shadow': 1,
'no-spaced-func': 1,
'no-trailing-spaces': 1,
// Allows Chai `expect` expressions
'no-unused-expressions': 0,
'no-var': 1,
'object-curly-spacing': [ 1, 'always' ],
'operator-linebreak': [ 1, 'after', { overrides: {
'?': 'before',
':': 'before'
} } ],
'padded-blocks': [ 1, 'never' ],
'prefer-const': 1,
'quote-props': [ 1, 'as-needed', { keywords: true } ],
quotes: [ 1, 'single', 'avoid-escape' ],
'react/jsx-curly-spacing': [ 1, 'always' ],
'react/jsx-no-bind': 1,
'react/jsx-space-before-closing': 1,
'react/no-did-mount-set-state': 1,
'react/no-did-update-set-state': 1,
'react/no-is-mounted': 1,
'react/prefer-es6-class': 1,
semi: 1,
'semi-spacing': 1,
'space-before-blocks': [ 1, 'always' ],
'space-before-function-paren': [ 1, 'never' ],
'space-in-parens': [ 1, 'always' ],
'space-infix-ops': [ 1, { int32Hint: false } ],
'space-unary-ops': [ 1, {
overrides: {
'!': true
}
} ],
'template-curly-spacing': [ 1, 'always' ],
'valid-jsdoc': [ 1, { requireReturn: false } ],
'wpcalypso/i18n-ellipsis': 1,
'wpcalypso/i18n-no-collapsible-whitespace': 1,
'wpcalypso/i18n-no-this-translate': 1,
'wpcalypso/i18n-no-variables': 1,
'wpcalypso/i18n-mismatched-placeholders': 1,
'wpcalypso/import-docblock': 1,
'wpcalypso/jsx-gridicon-size': 1,
'wpcalypso/jsx-classname-namespace': 1,
camelcase: 0, // REST API objects include underscores
'no-unused-expressions': 0, // Allows Chai `expect` expressions
}
};
28 changes: 9 additions & 19 deletions bin/pre-commit
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
#!/bin/sh

PATH="$PATH:/usr/local/bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

@nosolosw For those of us using GUI git apps like Tower.app, removing PATH causes commit to stop working as the app no longer has PATH info to run the commands in .git/hooks/pre-commit. Can you revert this line?

More info: https://www.git-tower.com/help/mac/faq-and-tips/faq#faq-hook-scripts

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure @raoulwegat , can you test #12548 works and green-lit it?


printf "\nBy contributing to this project, you license the materials you contribute under the GNU General Public License v2 (or later). All materials must have GPLv2 compatible licenses — see .github/CONTRIBUTING.md for details.\n\n"

# Make quick pass over config files on every change
Expand All @@ -16,21 +14,13 @@ pass=true

printf "\nValidating .jsx and .js:\n"

for file in ${files}; do
./node_modules/.bin/eslint --cache ${file}
if [ $? -ne 0 ]; then
printf "\033[31meslint Failed: %s\033[0m\n" "${file}"
pass=false
else
printf "\033[32meslint Passed: %s\033[0m\n" "${file}"
fi
done

printf "\neslint validation complete\n"

if ! $pass; then
printf "\n\033[41mCOMMIT FAILED:\033[0m Your commit contains files that should pass validation tests but do not. Please fix the errors and try again.\n\n"
exit 1
else
printf "\n\033[42mCOMMIT SUCCEEDED\033[0m\n\n"
./bin/run-lint $(git diff --cached --name-only client/ server/ test/ | grep ".jsx*$") -- --diff=index
linter_exit_code=$?
if [ ! 0 = "$linter_exit_code" ]
then
printf "\n"
printf "\033[41mCOMMIT ABORTED:\033[0m the linter reported some problems. If you are aware of them and it is OK, repeat the commit command with --no-verify to avoid this check. You may also want to execute the linter after the errors have been solved: ./bin/run-lint \$(git diff --cached --name-only client/ server/ test/) -- --diff=index"
printf "\n"
fi

exit $linter_exit_code
68 changes: 53 additions & 15 deletions bin/run-lint
Original file line number Diff line number Diff line change
@@ -1,21 +1,59 @@
#!/usr/bin/env node
const child_process = require( 'child_process' );

const path = require( 'path' );
var args = [ '--cache', '--quiet', '--ext=.js', '--ext=.jsx' ];
if ( process.argv.length > 2 ) {
args = args.concat( process.argv.slice( 2 ) );
const child_process = require( 'child_process' );

var argsESLint = [ '--cache', '--quiet', '--ext=.js,.jsx', '--format=json' ];
var argsESLines = [ ];

const markerIndex = process.argv.indexOf('--');
if ( ( process.argv.length > 2 ) && ( markerIndex > 2 )) {
// use -- as a marker for end of options,
// so any value that follows the marker is considered an option for ESLines.
if ( markerIndex > -1 ) {
argsESLint = argsESLint.concat( process.argv.slice( 2, markerIndex ) );
argsESLines = argsESLines.concat( process.argv.slice( markerIndex + 1 ) );
} else {
argsESLint = argsESLint.concat( process.argv.slice( 2 ) );
}
} else {
args = args.concat( [ '.' ] );
process.stdout.write( 'No files to lint\n' );
process.exit( 0 );
}

const results = child_process.spawnSync( path.join( '.', 'node_modules', '.bin', 'eslint' ), args );
const eslint = child_process.spawn( path.join( '.', 'node_modules', '.bin', 'eslint' ), argsESLint );
const eslines = child_process.spawn( path.join( '.', 'node_modules', '.bin', 'eslines' ), argsESLines );

if ( results.stdout ) {
process.stdout.write( results.stdout );
}
if ( results.stderr ) {
process.stderr.write( results.stderr );
}
process.on( 'exit', function() {
process.exit( results.status );
} );
eslint.stdout.on( 'data', ( data ) => {
eslines.stdin.write( data );
});
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a bit of irony in that these files don't adhere to our ESLint configuration 😄 Specifically, lacking space before closing paren. Also var below should be let.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is, indeed! :D And probably has to do with the fact that eslint, by default only lints js files and editors have a hard time giving you that info.

Copy link
Contributor

@oandregal oandregal Jan 16, 2017

Choose a reason for hiding this comment

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

Also: AFAIK, this was a script that originally only run in CircleCI and in dev machines and maybe and the time ES6 wasn't available for all those environments. Maybe that's the reason we do not lint the /bin directory. Just guessing.

If it is not a blocker, I'd prefer solving (and testing) this in another PR after this has been landed.


var eslintStdErr = 0;
eslint.stderr.on( 'data', ( data ) => {
eslintStdErr = 1;
process.stderr.write( data );
});

eslint.on('close', ( code ) => {
eslines.stdin.end();
});

eslines.stdout.on( 'data', ( data ) => {
process.stdout.write( data );
});

eslines.stderr.on( 'data', ( data ) => {
process.stderr.write( data );
});

eslines.on( 'close', ( code ) => {
// since the goal of eslines is to downgrade errors
// on non-modified lines, we can't count on eslint's
// exit code, but we assume that if eslint output
// something on its stderr, this means it encountered
// some other error
if ( eslintStdErr === 1 ) {
process.exit( 1 );
}
process.exit( code );
});
20 changes: 18 additions & 2 deletions npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@
"test-client:watch": "nodemon -e js,jsx --exec npm run test-client",
"test-server:watch": "nodemon -e js,jsx --exec npm run test-server",
"test-test:watch": "nodemon -e js,jsx --exec npm run test-test",
"lint": "bin/run-lint",
"lint": "bin/run-lint .",
"css-lint": "stylelint 'client/**/*.scss' --syntax scss"
},
"devDependencies": {
Expand All @@ -167,6 +167,7 @@
"esformatter-quotes": "1.0.3",
"esformatter-semicolons": "1.1.1",
"esformatter-special-bangs": "1.0.1",
"eslines": "0.0.10",
"eslint": "3.8.1",
"eslint-config-wpcalypso": "0.6.0",
"eslint-plugin-react": "6.4.1",
Expand Down