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

[EsLint] Add comma-dangle rule #14631

Closed
wants to merge 2 commits into from
Closed

[EsLint] Add comma-dangle rule #14631

wants to merge 2 commits into from

Conversation

rhoboat
Copy link

@rhoboat rhoboat commented Oct 27, 2017

Adds a comma-dangle rule, which you can read about.

Options:

Set it to only-multiline.

The only sane options are only-multiline and always-multiline. I'd be good with always-multiline, but it is a bit more strict, as there will be errors if there are no trailing commas on multiline arrays, objects, imports, and exports. However, with --fix, there's no issue with being bold and going with always-multiline. I just picked one. If people feel strongly and want to go with always-multiline, I'd be down, yo.

Functions caveat:

There's a setting for functions, which is recommended only if you're writing in ES2017+, which we are. This is because:

Trailing commas in function declarations and function calls are valid syntax since ECMAScript 2017

However, if I simply declare the one-liner string option:

"comma-dangle": ["error", "[only/always]-multiline"],

I think this means eslint will set "functions": "ignore" in the rule. The explanation in the linked docs is hard to interpret. See:

functions is set to "ignore" by default for consistency with the string option.

I think this is even if you set the string option to something other than never, as in the example documentation. So to force functions to be not-ignored, I think you have to use the object option (as I have in the PR), and specify what you want for functions.

If we prefer to ignore the functions, it means stuff like this could be annoying in diffs and code reviews. Code that requires multiline args for function declarations and invocations, i.e.:

function (reallyLongVariableNameThatAlsoHasADefaultWooHoo = [],
  anotherReallyLongVariableEpithetThatMayOrMayNotHaveADefault,
) {}

So I propose handling functions just like we do everything else, either the only- or always-multiline.

Discuss.

@rhoboat
Copy link
Author

rhoboat commented Oct 27, 2017

I can't select everyone in kibana as a reviewer, but I'd appreciate all eyes on this.

Copy link
Contributor

@kimjoar kimjoar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a separate commit with all the changes too? Just to see the impact this has. I'm not against this, but if it touches every file in the repo it might be a bit too much, as it might cause tons of merge problems with our ~100 PRs (or maybe there's a simple way to solve that? I haven't found a good solution before, but haven't thought too much about it)

@cjcenizal
Copy link
Contributor

@archanid If you want to commit the changes before publishing the package, you can do what I did, which was to just copy/paste the config into the node_modules config directly. You'll just have to remember to publish it later, and not forget like I did.

@weltenwort
Copy link
Member

Replacing the folder in node_modules with a symlink to the folder in packages is convenient for local development.

@kimjoar
Copy link
Contributor

kimjoar commented Oct 27, 2017

npm link in the eslint packages folder, then npm link {packagename} at the root should work too (it's basically the same as what Felix describes ^)

@cjcenizal
Copy link
Contributor

Well, sure, if you want to be all smart about it.

@Bargs Bargs removed their request for review October 30, 2017 23:01
@thomasneirynck thomasneirynck removed their request for review November 3, 2017 02:30
@rhoboat
Copy link
Author

rhoboat commented Nov 3, 2017

Cool, yeah I've been sitting on this for a while. I'll do that and ping when it's ready.

@elastic elastic locked and limited conversation to collaborators Nov 3, 2017
@azasypkin azasypkin removed their request for review November 10, 2017 14:13
@tylersmalley tylersmalley removed their request for review November 16, 2017 00:13
@jbudz jbudz removed their request for review November 30, 2017 03:55
@timroes timroes removed their request for review December 18, 2017 13:09
@rhoboat
Copy link
Author

rhoboat commented Jan 19, 2018

I think I'll abandon this in favor of using a prettier rule for this.

@rhoboat rhoboat closed this Jan 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants