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

Update eslint rules #3162

Closed
antoinerg opened this issue Oct 25, 2018 · 17 comments
Closed

Update eslint rules #3162

antoinerg opened this issue Oct 25, 2018 · 17 comments

Comments

@antoinerg
Copy link
Contributor

antoinerg commented Oct 25, 2018

Following @etpinard's #3158 (comment), we should probably update our linter syntax to reflect the current coding style.

In particular we should probably add "one-var": { "initialized": "never", "uninitialized": "consecutive" } and fix any possible errors.

Update: the "initialized" part got done in #3374

@antoinerg antoinerg changed the title Update lint style Update eslint rules Oct 25, 2018
@etpinard
Copy link
Contributor

etpinard commented Oct 26, 2018

We should also add the camelcase rule with the {"properties": "never"} option. I've seen a few community developers defining snake-case variables in their PRs.

Done in #3374

@alexcjohnson

This comment has been minimized.

@etpinard

This comment has been minimized.

@etpinard
Copy link
Contributor

One more from https://github.com/plotly/plotly.js/pull/3125/files#r227541345:

Maybe we could add https://eslint.org/docs/rules/no-extra-parens to our lintinting rules.

@etpinard
Copy link
Contributor

from #3020


consider enabling https://eslint.org/docs/rules/no-shadow - can save us from some very confusing bugs.

As @etpinard mentions in #3017:

That would require some work though, as no-shadow fails in more than 500 times in our codebase.

This was referenced Dec 21, 2018
@etpinard
Copy link
Contributor

Potential new rule: /*eslint quote-props: ["error", "as-needed"]*/

https://eslint.org/docs/rules/quote-props

which fits the style in most (but not all!) of our src files.

@etpinard
Copy link
Contributor

etpinard commented Jan 21, 2019

Potential new rule: /*eslint padded-blocks: ["error", "never"]*/

https://eslint.org/docs/rules/padded-blocks

DONE in: #3697

@etpinard
Copy link
Contributor

Maybe update our no-multiple-empty-lines rule down to {"max": 1}

@archmoj
Copy link
Contributor

archmoj commented Apr 10, 2019

We previously started to dislike else statements sitting on new lines and tried to fix them in other PRs.
lint-else

To put all else and catch statements inline with the closing curly braces, I suggest we use "brace-style": [2, "1tbs", {"allowSingleLine": true}], instead of "brace-style": [0, "stroustrup", {"allowSingleLine": true}], then run npm run lint -- --fix on a separate PR (possibly now that we have only a few PRs open).
We could then fix just a few edge cases where there is a comment on top of an else statement.

DONE in: #3757

@etpinard

This comment has been minimized.

@archmoj
Copy link
Contributor

archmoj commented Apr 17, 2019

Potential rule: https://eslint.org/docs/rules/func-names

@etpinard
Copy link
Contributor

Potential rule: https://eslint.org/docs/rules/max-statements-per-line

@archmoj
Copy link
Contributor

archmoj commented May 22, 2019

Potential rule: https://eslint.org/docs/rules/max-statements-per-line

I am a fan of it.
Also we may limit the number of characters on each line using: https://eslint.org/docs/rules/max-len.

@archmoj
Copy link
Contributor

archmoj commented Jun 11, 2019

Potential rule: https://eslint.org/docs/rules/object-curly-spacing
eslint object-curly-spacing: ["error", "always"]

@etpinard
Copy link
Contributor

etpinard commented Jul 23, 2019

https://eslint.org/docs/rules/no-prototype-builtins is now turned on by default in eslint v6, we turned it off in e61ee25 (as it made npm run lint fail in about ~20 places) but it might be nice turn it back on at some point

@archmoj
Copy link
Contributor

archmoj commented Aug 6, 2019

Potential rule: https://eslint.org/docs/rules/global-require

@gvwilson
Copy link
Contributor

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson

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

No branches or pull requests

5 participants