Getting eslint-config-eslint in line with coding standards

[platinumazure]

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]

@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]

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

[platinumazure]

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]

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]

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
  • (Edit) Enable one-var, per @ilyavolodin's suggestion
• 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]

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

[ilyavolodin]

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

[platinumazure]

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

[BYK]

👍

[nzakas]

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]

@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]

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

[BYK]

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]

@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]

@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]

@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]

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]

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

[nzakas]

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]

@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.

[nzakas]

@platinumazure confirmed

[platinumazure]

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

[mysticatea]

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]

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

[platinumazure]

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]

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]

@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]

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

[alberto]

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

[nzakas]

👍 to enabling no-console.

[platinumazure]

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]

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]

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]

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

[nzakas]

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

[gyandeeps]

@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]

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