Enable `object-curly-spacing` and `comma-dangle` on the ESLint codebase #7725

Closed
not-an-aardvark opened this Issue Dec 8, 2016 · 9 comments

Projects

None yet

6 participants

@not-an-aardvark
Member
not-an-aardvark commented Dec 8, 2016 edited

I think it would be nice to enable object-curly-spacing and comma-dangle on the ESLint codebase. A lot of our code is inconsistent about this (sometimes it's even inconsistent in the same object, e.g. { message: "foo"}).

I ran the rules on the existing codebase to determine which options we should use:

$ eslint --rule 'comma-dangle:[error, never]' 'lib/**/*.js' 'tests/**/*.js' 'bin/**/*.js' --rulesdir lib/internal-rules/ | grep 'problems'
✖ 227 problems (227 errors, 0 warnings)
$ eslint --rule 'comma-dangle:[error, always-multiline]' 'lib/**/*.js' 'tests/**/*.js' 'bin/**/*.js' --rulesdir lib/internal-rules/ | grep 'problems'
✖ 13251 problems (13251 errors, 0 warnings)

It looks like we almost always omit the trailing comma, so I think we should enable comma-dangle: never.

$ eslint --rule 'object-curly-spacing: [error, always]' 'lib/**/*.js' 'tests/**/*.js' 'bin/**/*.js' --rulesdir lib/internal-rules/ | grep 'problems'
✖ 9272 problems (9272 errors, 0 warnings)
eslint --rule 'object-curly-spacing: [error, never]' 'lib/**/*.js' 'tests/**/*.js' 'bin/**/*.js' --rulesdir lib/internal-rules/ | grep 'problems'
✖ 16060 problems (16060 errors, 0 warnings)

It looks like we usually include curly spacing, so I think we should enable object-curly-spacing: always.

I'll probably create a PR to add these in the next few days if no one has any objections. I just wanted to create this issue first to give some advance notice in case anyone is against enabling these rules, since it seems like we generally end up merging PRs like that fairly quickly to avoid merge conflicts.

@platinumazure
Member
platinumazure commented Dec 8, 2016 edited

👍 to comma-dangle: "never", object-curly-spacing: "always". I can live with comma-dangle: "always-multiline" as well.

@ilyavolodin
Member

I think our styleguide requires comma-dangle: ["error", "never"].

@platinumazure
Member

@ilyavolodin I didn't see it when I took a quick look. What section should I be looking at?

@mysticatea
Member

Before, comma-dangle had been applied to our codebase, but it was dropped from eslint:recommended at 3.0.0.

It's fine comma-dangle: ["error", "never"] for implementations but I think comma-dangle: ["error", "always-multiline"] is better for tests because we can get better diff.
(though my personal preference is always-multiline at all 😃)

@not-an-aardvark
Member

I would be fine with either comma-dangle: [error, never] or comma-dangle: [error, always-multiline]

@vitorbal
Member
vitorbal commented Dec 9, 2016

My personal preference for comma-dangle is also always-multiline, but I am 👍 for adding it either way, for consistency's sake.
I'm also 👍 for object-curly-spacing: "always"

@kaicataldo
Member

I don't have a huge preference for comma-dangle, but I do like the idea of nicer Git diffs :)

@not-an-aardvark not-an-aardvark added a commit that referenced this issue Dec 14, 2016
@not-an-aardvark not-an-aardvark Chore: enable object-curly-spacing on ESLint codebase (refs #7725) 263def8
@not-an-aardvark not-an-aardvark added a commit that referenced this issue Dec 14, 2016
@not-an-aardvark not-an-aardvark Chore: enable object-curly-spacing on ESLint codebase (refs #7725) dacb42b
@kaicataldo
Member
kaicataldo commented Jan 10, 2017 edited

Sounds like the only thing left to hash out here is how we want to configure comma-dangle. Seems like most of us would be fine with "never" or "always-multiline". Since we have one vote strictly in favor of "never", I propose we go with "never" (which is what our codebase currently follows anyway).

@vitorbal
Member
@not-an-aardvark not-an-aardvark added a commit that referenced this issue Jan 11, 2017
@not-an-aardvark not-an-aardvark Chore: Enable comma-dangle on ESLint codebase (fixes #7725) 7cd5d6a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment