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

ghost · 2016-07-21T13:17:00Z

We are migrating old code to the new standards (see janitorial efforts at #8562). In the meantime, the new code is failing to adhere to our current code-style because there are rules set as warnings yet.

Until now, they could not be set to errors because doing so will break the git hooks and our CI tools. This PR aims to address that problem by only reporting errors in lines modified, instead of the whole code base. With this approach, we can enforce that all new code is always written following the up-to-date conventions, which will help the migration effort to be completed sooner.

The gist of this PR is just piping commands for several contexts:

eslint -f json | eslines

Caveats

Editor integration has proven harder than expected, and, at this point, it's not a goal to aim for it. This means that in editors all rules will be reported as errors.

For editors that allow calling custom commands for linting, there may be a way out and a solution may be found. I'm happy to help with this. Yet, as it needs to be considered in a case-by-case basis and it is time-consuming I'd rather do it after this PR is merged/aborted.

How to test locally

git clone [email protected]:Automattic/wp-calypso.git
git checkout -b add/eslint-formatters-calypso-testcase origin/add/eslint-formatters-calypso-testcase
npm install
make githooks

Test that it works:

open client/components/web-preview/index.jsx and remove the spaces after and before the parens at line 167. Do not introduce any other change.
git add client/components/web-preview/index.jsx
git commit: expected result is that commit hook will report an error in the line modified and won't let you commit (notice that the same error in lines not modified is considered a warning)

Test that it doesn't prevent you from commiting (if the errors are in lines not modified):

open client/components/web-preview/index.jsx and remove all debug statements and the debug importing. Do not introduce any other change.
git add client/components/web-preview/index.jsx
git commit: expected result is that commit hook will not report any error.

nb · 2016-07-21T18:29:48Z

If we are adding the whole project to Calypso, why not just make it a dependency? I know we chatted about having the code as part as Calypso, but in this case it should live in lib/ somewhere and wouldn't need licenses, package.json and things like this.

nb · 2016-07-21T18:34:49Z

bin/run-lint

+const argsBranchName = [ 'rev-parse', '--abbrev-ref', 'HEAD' ];
+const branchName = child_process.spawnSync( 'git', argsBranchName ).stdout.toString().trim();
+
+if ( branchName === 'master' ) {


Is there a chance we can move this logic out of the run-lint script and somehow into the eslines code? I would like to keep running eslint simple, so that people can easily do it outside of Calypso. If we always need those format arguments, it will be much harder to integrate with editors.

Thanks for the suggestion! At the moment, what I have done is to create a new formatter at eslint-formatters-calypso: it is only a wrapper for deciding which formatter to return depending on the git branch. I would need to check how different editors call eslint, but I guess passing a formatter like in eslint -f eslines . should be possible. However it may be, moving the logic inside eslint-formatters-calypso is a good idea.

As an aside: eslines has been rebranded to a more meaningful name: eslint-formatters-calypso, as its responsibilities have grown and changed. eslines is now the name of the smart and wrapper formatter inside that package.

Why does it need to be a formatter? Can’t it be an eslint plugin, so that we can include it in the configuration?

TL;DR:

It can't be a plugin, but we need something more than a formatter (at least, the ability to control exit codes). This is something we can easily do by piping eslint . | eslines.

Take advantage of this work from the editors seems easy, as they usually execute npm run lint for linting or allow to configure a command manually - I need to research more on this, though.

# More context about formatters VS plugins

An ESLint plugin bundles a set of rules, environment variables and processors. An ESLint formatter is something you pass as an argument to the CLI and there is no way to make it play together with the config file (see code - currentOptions.format is the command line option --format). The ESLint team has already had some discussions around this: move formatters into plugins or have the ability to pass options to formatters.

# More than a formatter

There are situations where people needs to configure the formatter (for example, to configure the colors or the file paths for a specific environment) or it needs to do more than an ESLint formatter is capable of.

To work around this, sometimes, people encapsulates the call to ESLint in its own script (eslint-nibble, eslint-pretty).

Other approaches that I have seen in practice while using ESLint directly are:

The recommended way is:

eslint -f json . | formatter --your-options

This is something we can benefit from as it would allow us to control the exit code (actually, we can already do this, as before it was migrated to formatter this is the approach I was taking).

The common way to pass info to the formatter is by using environment variables or a config file (we use also the config file).

There is some other hacky way that depends on the fact that ESLint uses optionator for parsing options from the command line. This is used by eslint-friendly-formatter. The way this works is:

ESLint uses optionator to parse command line arguments.

Optionator provides an API to do optionator.parse(process.argv) (see ESLint code) that returns an object like {opt1: 2, opt2:3, _: ['positional-param-1', 'positional-param-2']} with keys being the options and any other positional parameter added in the _ array.

ESLint uses the contents in the _ array as the input files (but discards any file that does not exist).

Optionator happen to follow the bash convention of using -- as a marker for end of options so any value that follows the marker is considered a positional parameter. example of the hack:

command --opt1 2 --opt2 3 -- file-to-process-1 file-to-process-2 --opt3

for this command, optionator.parse(process.argv) would return{opt1: 2, opt2:3, _: ['file-to-process-1', 'file-to-process-2', '--opt3']}.

eslint-friendly-formatter takes advantage of this hack (ESLint would not process --opt3 as that file does not exists) to retrieve the command line options after the -- marker and use them for its own configuration.

seear · 2016-07-25T14:42:50Z

For the master branch, it would only break the build for ESLint parsing errors.

Something I didn't understand very well from the previous discussion around this—do we ever run CircleCI on master?

blowery · 2016-07-25T17:31:38Z

Something I didn't understand very well from the previous discussion around this—do we ever run CircleCI on master?

@seear yup, on every commit to master. Right now failures only get reported to project owners via email.

ghost · 2016-07-27T10:47:54Z

@seear @blowery CircleCI build runs on every merge (which may include more than one commit, depending on how the topic branch is merged into master - squashed or not). See for example these two random builds: one commit and a merge with several commits.

seear · 2016-07-27T21:26:16Z

bin/eslint-formatters-calypso/src/lib/differ.js

+        *
+        * ex: @@ -65,3 +65,3 @@ module.exports = {
+        *
+        * 65 is the starting point, 3 the number of lines affected,


3 the number of lines affected

I think the counts for a hunk (the number after the comma) includes the context lines, not just the modified lines—see example below. So we will be including the context lines from the diff in the lint. Is that what we want? (Maybe it is.)

Ignore above comment, just spotted the -U0 passed to the diff command in git-diff 😄

ghost · 2016-07-29T17:08:26Z

@nb In response to #6945 (comment). Well, you already know I think it makes sense to distribute this as an independent repo/package. In importing this into Calypso repo, I asked myself a few questions:

where should it be placed? bin/ was the directory that made the most sense to me, as other building scripts are there. I would be happy to move it to other place - but, currently, I don't see any other place better than this. Any suggestion?
what files shall remain? I was deleting the LICENSE at the same time you were commenting, as I realized that should not be there. As for the other files (package.json, etc): I left them there because they are useful for development (to have an isolated npm run test for this package, for example).
how to call it? I was reviewing if we could import this as an ESLint plugin, but that is not possible. An argument that can be done is whether it should be imported as a npm local package and use it by calling ./node_packages/eslint-formatter-calypso/ or call it directly from the path ./bin/eslint-formatter-calypso. I don't have a strong opinion about this. If I chose the second option is because it seemed less intrusive with the current state of things - but I would be happy to change it. Do you think it would be better to import it as a npm package? Do you see any other option?

ghost · 2016-07-31T21:26:10Z

@nb this is ready for a second round. The git hooks pre-push and pre-commit have been updated to use eslines formatter and the feedback for the first round processed. The explanation in the PR has been updated too.

If this approach generates consensus, I would like to merge this PR into master after resolving any issues that may appear within this second round. Then, I would address anything left (for example: editors integration with ESLint, etc) within other PRs.

ghost · 2016-08-13T12:42:18Z

@nb this branch has been rebased. I have also updated the pre-commit and pre-push git hooks to be more GUI friendly.

ghost · 2016-08-16T16:49:09Z

Reverted the commits introduced for test-case this PR in CircleCI. This is ready to be merged into master.

nb · 2016-09-08T00:04:36Z

bin/eslint-formatters-calypso/package.json

+    "test-differ": "./node_modules/.bin/tape tests/test-differ.js | ./node_modules/.bin/faucet",
+    "test-parsing-errors": "./node_modules/.bin/tape tests/test-parsing-errors.js | ./node_modules/.bin/faucet"
+  },
+  "dependencies": {


Do we need to run npm install in this directory in order for the command-line tool to work?

oandregal · 2017-02-21T16:47:38Z

Current state: editor integration is a blocker. We need the editors to run a custom command for linting so we can pipe eslines to the result of eslint, as we did for CI and git hooks. I've tested Atom and WebStorm and they don't allow this.

So, either we provide a custom eslint that wraps the eslines utility or editors will report all rules as errors because we cannot filter them with eslines. As per previous conversations, neither alternative is good enough, so I'm giving by unconscious brain some time to come up with something else.

The linting task will report: - only parsing errors, when running on master branch - for other branches, any ESLint error in * lines modified within that branch * any linted files for the rule no-unused-vars pre-commit githook and CircleCI were modified to fail if linter reports any error

oandregal · 2017-03-21T12:32:17Z

I was about merging this, but on rebasing and shrinkwrapping again, I found that some tests were not passing and the cause was unrelated to this PR. I prefer separate concerns and solve these issues in a separate PR before this is merged. Proposal to fix it in #12356

After that is merged, this needs a rebase.

The gist of this PR is: eslint -f json | eslines With these changes in place, the linting task will report: * only parsing errors, when running on master branch. * for other branches, any ESLint error in * lines modified within that branch * any linted files for the rule no-unused-vars

They were removed in #6945 inadvertently.

…#12446) They were removed in #6945 inadvertently.

raoulwegat · 2017-03-27T11:03:39Z

bin/pre-commit

@@ -1,7 +1,5 @@
 #!/bin/sh

-PATH="$PATH:/usr/local/bin"


@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

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

matticbot added the OSS Citizen label Jul 21, 2016

ghost self-assigned this Jul 21, 2016

ghost mentioned this pull request Jul 21, 2016

Teach CircleCI to run eslint for files modified in the topic branch #6625

Closed

nb reviewed Jul 21, 2016
View reviewed changes

seear reviewed Jul 27, 2016
View reviewed changes

ghost changed the title ~~Teach CircleCI to be smarter about linting~~ Changing the linting strategy Jul 31, 2016

ghost mentioned this pull request Aug 6, 2016

Fix no-unused-vars issues #7315

Merged

ghost force-pushed the add/eslint-formatters-calypso-testcase branch from 863296a to f9d4a6c Compare August 13, 2016 12:40

lancewillett added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 2, 2016

nb reviewed Sep 8, 2016
View reviewed changes

oandregal removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Feb 21, 2017

oandregal force-pushed the add/eslint-formatters-calypso-testcase branch from 8d94e72 to 25b0c00 Compare March 9, 2017 13:51

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 9, 2017

oandregal changed the title ~~Changing the linting strategy~~ Set all ESLint rules as errors while preventing hooks/CI from failing Mar 9, 2017

oandregal changed the title ~~Set all ESLint rules as errors while preventing hooks/CI from failing~~ Set ESLint rules as errors while preventing hooks/CI from failing Mar 9, 2017

oandregal force-pushed the add/eslint-formatters-calypso-testcase branch 2 times, most recently from 8098f1e to a522b04 Compare March 21, 2017 11:26

oandregal mentioned this pull request Mar 21, 2017

Update/shrinkwrap #12356

Merged

oandregal added [Status] Blocked / Hold [Status] Needs Rebase and removed [Status] In Progress labels Mar 21, 2017

oandregal force-pushed the add/eslint-formatters-calypso-testcase branch from a522b04 to 990ad45 Compare March 22, 2017 11:50

oandregal force-pushed the add/eslint-formatters-calypso-testcase branch from 990ad45 to e499569 Compare March 22, 2017 12:45

oandregal merged commit a5889f9 into master Mar 22, 2017

oandregal deleted the add/eslint-formatters-calypso-testcase branch March 22, 2017 12:52

oandregal removed [Status] Blocked / Hold [Status] Needs Rebase labels Mar 22, 2017

tyxla mentioned this pull request Mar 22, 2017

Add back eslint max-len of 140 characters #12415

Merged

aduth mentioned this pull request Mar 22, 2017

Framework: Define all ESLint rules as errors #8562

Closed

oandregal added a commit that referenced this pull request Mar 23, 2017

Add back no-restricted-modules and no-restricted-imports ESLint rules

a6ed3e0

They were removed in #6945 inadvertently.

This was referenced Mar 23, 2017

Add back no-restricted-modules and no-restricted-imports ESLint rules #12446

Merged

Check module import validity with eslint-plugin-import #12303

Closed

oandregal pushed a commit that referenced this pull request Mar 23, 2017

Add back no-restricted-modules and no-restricted-imports ESLint rules (…

ea68492

…#12446) They were removed in #6945 inadvertently.

oandregal mentioned this pull request Mar 23, 2017

Enable accessibility rules for eslint #12027

Closed

raoulwegat reviewed Mar 27, 2017

View reviewed changes

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ghost commented Jul 21, 2016 •

edited by oandregal

Loading

nb commented Jul 21, 2016

nb Jul 21, 2016

ghost Jul 22, 2016

nb Jul 22, 2016

ghost Jul 29, 2016 •

edited by ghost

Loading

seear commented Jul 25, 2016

blowery commented Jul 25, 2016

ghost commented Jul 27, 2016

seear Jul 27, 2016

seear Jul 27, 2016

ghost commented Jul 29, 2016

ghost commented Jul 31, 2016

ghost commented Aug 13, 2016

ghost commented Aug 16, 2016

nb Sep 8, 2016

oandregal commented Feb 21, 2017 •

edited

Loading

oandregal commented Mar 21, 2017 •

edited

Loading

raoulwegat Mar 27, 2017

oandregal Mar 27, 2017

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

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

Conversation

ghost commented Jul 21, 2016 • edited by oandregal Loading

Caveats

How to test locally

nb commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 29, 2016 • edited by ghost Loading

Choose a reason for hiding this comment

# More context about formatters VS plugins

# More than a formatter

seear commented Jul 25, 2016

blowery commented Jul 25, 2016

ghost commented Jul 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 29, 2016

ghost commented Jul 31, 2016

ghost commented Aug 13, 2016

ghost commented Aug 16, 2016

Choose a reason for hiding this comment

oandregal commented Feb 21, 2017 • edited Loading

oandregal commented Mar 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Jul 21, 2016 •

edited by oandregal

Loading

ghost Jul 29, 2016 •

edited by ghost

Loading

oandregal commented Feb 21, 2017 •

edited

Loading

oandregal commented Mar 21, 2017 •

edited

Loading