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

Getting eslint-config-eslint in line with coding standards #5188

Closed
platinumazure opened this Issue Feb 8, 2016 · 44 comments

Comments

Projects
None yet
8 participants
@platinumazure
Copy link
Member

platinumazure commented Feb 8, 2016

Per discussion in chat, the goal of this issue is to document (and eventually implement) changes to the Coding Standards and/or eslint-config-eslint to bring both in line as much as possible.


Actionable items:

  • Enable space-in-parens to enforce no spaces in parentheses
  • Enable quote-props (as-needed) to enforce unquoted properties
  • Enable lines-around-comment to require blank lines before line and block
  • Enable vars-on-top to ensure variables are declared at the top of their containing scope (Note: Might be removed)
  • Enable one-var, per @ilyavolodin's suggestion (Note: Might be removed) Using one-var-declaration-per-line
  • Enable one-var-declaration-per-line to enforce declaring one variable per line
  • Enable strict to enforce strict mode at the desired scope
  • Enable newline-after-var to enforce blank line between variable declarations and first statement
  • Enable no-console
  • Enable valid-jsdoc if not enabled, use preferType to ensure lowercase builtin types per guidelines.
@eslintbot

This comment has been minimized.

Copy link

eslintbot commented Feb 8, 2016

@platinumazure Thanks for the issue! If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Feb 9, 2016

And what's the approach? We need some seed information on this issue so people know what the direction is.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 4, 2016

I'm not sure, to be honest. I was hoping to gather some examples.

I just found one: @nzakas commented about wanting JSDoc parameter annotations not to have punctuation between the parameter name and the description here. We have no way to enforce that in ESLint right now, but that would be an example of a style preference that we aren't enforcing and theoretically could.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 6, 2016

Okay, I've gone through the Code Conventions document. I've put together a list of each section of that document, as well as whether we are enforcing it via our own ESLint configuration. My findings are here.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 10, 2016

Apologies for the delay.

Here's what I think is actionable now:

  • Enable space-in-parens to enforce no spaces in parentheses
  • Enable quote-props to enforce unquoted properties
  • Enable lines-around-comment to require blank lines before line and block comments
  • Enable vars-on-top to ensure variables are declared at the top of their containing scope
  • Enable one-var-declaration-per-line to enforce declaring one variable per line
  • Enable strict to enforce strict mode at the desired scope
  • Enable newline-after-var to enforce blank line between variable declarations and first statement

I would need to re-analyze my list to figure out what could be fixable with rule development (either as a core rule, if sufficiently general, or as a custom rule for the ESLint repository).

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Mar 11, 2016

Seems reasonable to me. @eslint/eslint-team thoughts?

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Mar 11, 2016

If we are enabling vars-on-top we should probably enable one-var as well. Otherwise sounds good to me.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 11, 2016

@ilyavolodin Good call, I've edited my previous comment to include your suggestion.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Mar 12, 2016

👍

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Mar 12, 2016

Looking over this again, I fear that vars-on-top and one-var would both require massive changes, so I'm a bit concerned about that. Those can't be autofixed, so it would be a huge pain to make those changes.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 12, 2016

@nzakas Acknowledged, but please do bear in mind that the original goal of this issue is to bring ESLint config in line with your stated coding convention preferences. So for each of the discrepancies I've identified, either we can tighten ESLint config to match the code convention, or we can loosen the coding convention to match the reality of ESLint's style. I don't think we should do nothing in these cases, because it could cause confusion.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Mar 12, 2016

Just checked. Yes, vars-on-top alone would be 838 errors in our codebase. That might be a bit too much.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Mar 12, 2016

I've dealt with 2k+ errors when doing this: disqus/heka-node@a1de77e

You can't scare me with 800 :D - Joking aside, I'm willing to give a hand.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 12, 2016

@ilyavolodin @nzakas Also I'd be willing to clean up large error chunks.

I want to ask about next steps. Should I file separate issues for each of the rules I've listed? Or is it sufficient for PRs to "ref" this issue until the last one?

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Mar 12, 2016

@platinumazure remember, we can define the relationship we want. One way to bring the conventions into alignment with the rules is to change the rules, another is to change the conventions. Both options are at our disposal, so it's really a question of what the team prefers as a whole.

Next step is to get consensus from the team about these last two rules. After that, I'd just use this one issue for multiple PRs with "refs".

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 13, 2016

@nzakas Cool. And regarding actions we can take, I'm glad we're on the same page:

@nzakas Acknowledged, but please do bear in mind that the original goal of this issue is to bring ESLint config in line with your stated coding convention preferences. So for each of the discrepancies I've identified, either we can tighten ESLint config to match the code convention, or we can loosen the coding convention to match the reality of ESLint's style.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 17, 2016

Okay, I've started work on a couple of the rules I called out above.

  • In progress, straightforward:
    • space-in-parens
  • Questions:
    • quote-props: Do we want "as-needed" or "consistent"? We usually quote keys of event handlers (i.e., node types) in rules, which are unnecessary (unless using :exit events). "consistent" seems appropriate but even that has a decent number of errors. "consistent-as-needed" could work too, but I do like the consistency of quoted rule event handler keys even when they're not strictly needed. Or does the convention we want to enforce really not fit with quote-props?
    • lines-around-comment: Just want to confirm that the standard really is that there should always be a line break before the comment, even at the start of a block? The Code Conventions do have a few examples but I want to know if @nzakas still values that highly enough that it should be enforced.
@BYK

This comment has been minimized.

Copy link
Member

BYK commented Mar 19, 2016

Re quote-props: my preference is "consistent-as-needed".

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Mar 19, 2016

I think "as-needed" is what I intended. I can't remember why I started doing node types in quotes.

For lines-around-comment: yes, especially at the start of a block.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 19, 2016

@nzakas @BYK I agree the coding conventions docs seem to straightforwardly indicate as-needed. But we've diverged wildly from that. @nzakas Just to confirm, you do want as-needed, for sure?

And for lines-around-comment, okay, I will add space at the start of blocks.

platinumazure added a commit to platinumazure/eslint that referenced this issue Mar 19, 2016

Update: Enforce coding conventions via ESLint (refs eslint#5188)
Dogfood space-in-parens and lines-around-comment rules in ESLint repo.

platinumazure added a commit to platinumazure/eslint that referenced this issue Mar 19, 2016

Update: Enforce repo coding conventions via ESLint (refs eslint#5188)
Dogfood space-in-parens and lines-around-comment rules in ESLint repo.
@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Mar 22, 2016

@platinumazure confirmed

@nzakas nzakas added accepted and removed evaluating labels Mar 22, 2016

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Mar 30, 2016

@gyandeeps Edited the initial post with what I believe the list should look like. Please let me know if I missed something. Thanks!

@mysticatea

This comment has been minimized.

Copy link
Member

mysticatea commented Apr 2, 2016

Just my feeling, I hate vars-on-top style...

By the way, Node v6 will be released soon, and we have a plan to drop supports of Node 0.x.
After that, I guess we use no-var rule. The let declarations are block-scoped, so I think we will get worse upgrade path if we apply vars-on-top now.

@BYK

This comment has been minimized.

Copy link
Member

BYK commented Apr 2, 2016

I'm with @mysticatea - I don't think vars-on-top is a good practice to have.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Apr 7, 2016

Any objections if I remove vars-on-top from my list?

If everyone is on board with it, I will then either file a PR in eslint.github.io or ask @nzakas to do so, in order to remove that stipulation from the coding guidelines.

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Apr 7, 2016

Fine by me. I would also add no-console to the list, although it's not a coding standard, we just had to do a patch release due to the left-over console statement. Feels like a good rule to enable.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Apr 7, 2016

@ilyavolodin I would imagine we use it extensively in CLI- how would you
recommend handling that case?
On Apr 7, 2016 4:21 PM, "Ilya Volodin" notifications@github.com wrote:

Fine by me. I would also add no-console to the list, although it's not a
coding standard, we just had to do a patch release due to the left-over
console statement. Feels like a good rule to enable.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#5188 (comment)

@ilyavolodin

This comment has been minimized.

Copy link
Member

ilyavolodin commented Apr 8, 2016

Disabling it with comments in the places where it's actually needed should be find, I think.

@alberto

This comment has been minimized.

Copy link
Member

alberto commented Apr 8, 2016

If we are not enabling vars-on-top, we should also remove one-var from the list.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Apr 8, 2016

👍 to enabling no-console.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Apr 14, 2016

Okay, clearly I didn't know what I was talking about when I said I would do quote-props. That's back on the table.

On the other hand, I am about to start no-console.

platinumazure added a commit to platinumazure/eslint that referenced this issue Apr 14, 2016

platinumazure added a commit to platinumazure/eslint that referenced this issue Apr 16, 2016

gyandeeps added a commit that referenced this issue Apr 23, 2016

platinumazure added a commit to platinumazure/eslint that referenced this issue Apr 23, 2016

gyandeeps added a commit that referenced this issue Apr 23, 2016

Merge pull request #5849 from platinumazure/no-console
Fix: Enable no-console rule in eslint-config-eslint (refs #5188)

nzakas added a commit that referenced this issue Apr 23, 2016

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Jul 8, 2016

Updated my list-- I think one-var won't happen due to one-var-declaration-per-line being enabled here. So the question becomes, do we implement vars-on-top or nix it? (If the latter, I'll send a PR to modify the style guide.)

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Jul 8, 2016

Should we enchance the valid-jsdoc config to also check for types using the option preferType.
So use boolean and not Boolean, etc?
I know I have seen @nzakas point these things out in PR.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Jul 8, 2016

@gyandeeps Absolutely yes, was just about to come back and add that to the list. Glad you agree.

@nzakas

This comment has been minimized.

Copy link
Member

nzakas commented Jul 8, 2016

@gyandeeps Yes! I'll do that now. :)

@nzakas nzakas added chore and removed enhancement labels Jul 8, 2016

nzakas added a commit that referenced this issue Jul 8, 2016

gyandeeps added a commit that referenced this issue Jul 8, 2016

@gyandeeps

This comment has been minimized.

Copy link
Member

gyandeeps commented Aug 10, 2016

@eslint/eslint-team I think we can close this issue, since we have covered everything.
we dont have to cover vars-on-top since we use let and const now.

@platinumazure

This comment has been minimized.

Copy link
Member Author

platinumazure commented Aug 10, 2016

Fine by me. Closing. Thanks everyone for your help!

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.