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

Framework: Update ESLint configuration to extend shared config #6543

Merged
merged 1 commit into from
Jul 6, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Jul 5, 2016

This pull request seeks to update our ESLint configuration to extend a shareable Calypso configuration published as eslint-config-wpcalypso (repository).

Implementation notes:

As a shareable configuration, the base rules should all be defined as errors (specifically to ensure new projects can properly catch these errors before they're introduced). Because Calypso has many lingering warnings, many of the rules have been overridden in our specific configuration to change their severity. I've added a comment noting this, with the hope that these overrides can be removed over time as issues are resolved.

Testing instructions:

There should be no usability impact from these changes.

Verify that linting produces the same results between master and update/eslint-config-external branches.

Test live: https://calypso.live/?branch=update/eslint-config-external

@aduth aduth added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 5, 2016
@nylen nylen force-pushed the update/eslint-config-external branch from 00345a8 to d9f1e1a Compare July 6, 2016 03:07
@nylen
Copy link
Contributor

nylen commented Jul 6, 2016

d9f1e1a is amended to remove the unrelated changes to the shrinkwrap file.

With this PR there is no more vars-on-top rule - it should be added to eslint-config-wpcalypso. Other than that, the lint results are the same.

How did you test the lint results? The cleanest way I was able to do it was by modifying bin/run-lint to remove the eslint --quiet flag.

@nylen nylen added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 6, 2016
@aduth
Copy link
Contributor Author

aduth commented Jul 6, 2016

d9f1e1a is amended to remove the unrelated changes to the shrinkwrap file.

Left comment in related conversation at #6539 (comment). I think it's fine to keep the amended commit.

With this PR there is no more vars-on-top rule

That's an eye for detail! This was actually an intended omission, one which I had meant to mention in the original comment. The reason I removed it is because it seems redundant given that we also enable the no-var rule.

How did you test the lint results?

Might not be exactly the same as bin/run-lint, but I ran eslint --ext .js,.jsx client/ server/ test (./node_modules/.bin/eslint if not installed globally).

Looking at bin/run-lint, without arguments it might also run against node_modules, which would be undesirable. Wonder if we should have a default there.

@aduth aduth added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Jul 6, 2016
@nylen
Copy link
Contributor

nylen commented Jul 6, 2016

Ah OK, that makes sense. Conflicts with #6539, otherwise good to go 👍

It looks like eslint skips node_modules unless explicitly told not to.

@nylen nylen added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jul 6, 2016
@aduth aduth force-pushed the update/eslint-config-external branch from d9f1e1a to facb9bc Compare July 6, 2016 19:59
@aduth aduth force-pushed the update/eslint-config-external branch from facb9bc to 091dc7d Compare July 6, 2016 20:11
@aduth
Copy link
Contributor Author

aduth commented Jul 6, 2016

It looks like eslint skips node_modules unless explicitly told not to.

Nice, good to know.

Per the merging of #6539, I separately published v0.2.0 of eslint-config-wpcalypso to drop the wpcalypso/no-lodash-import rule there (changelog), and bumped the dependency in this pull request.

@aduth aduth merged commit dc11a6a into master Jul 6, 2016
@aduth aduth deleted the update/eslint-config-external branch July 6, 2016 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants