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

Add ESLint comma-dangle rule #741

Merged
merged 1 commit into from
May 11, 2017
Merged

Add ESLint comma-dangle rule #741

merged 1 commit into from
May 11, 2017

Conversation

nylen
Copy link
Member

@nylen nylen commented May 9, 2017

Enforce trailing comma syntax for JavaScript arrays and objects:

return [
  'nice',
  'simple',
  'keys',
];
return {
  not: {
    so: {
      simple: {
        keys: {},
      },
    },
  },
};

Closes #612 - more discussion over there.

@aduth
Copy link
Member

aduth commented May 9, 2017

Hoping this was a simple eslint --fix, because recent shuffling caused a number of conflicts 😬

@aduth
Copy link
Member

aduth commented May 9, 2017

I know it's a style preference and all, but looking through the changes, I just can't help but feel that all the extra noise is not worth the insignificant benefit it brings.

@nylen
Copy link
Member Author

nylen commented May 9, 2017

Hoping this was a simple eslint --fix

Yes :)

I know it's a style preference and all, but looking through the changes, I just can't help but feel that all the extra noise is not worth the insignificant benefit it brings.

I think this has a decent amount of practical benefit beyond just being a style preference. Quoting from #612:

From the WordPress coding standards for PHP:

Note the comma after the last array item: this is recommended because it makes it easier to change the order of the array, and makes for cleaner diffs when new items are added.

Scattered throughout the JavaScript docs numerous times and to paraphrase quite heavily is:

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript
These are for consistency between the PHP and JavaScript files in the WordPress codebase.

Also, part of the reason you're seeing so much churn here is simply that we have a whole lot of code :)

@aduth
Copy link
Member

aduth commented May 9, 2017

Sorry, I didn't mean to suggest the benefits are purely cosmetic. I understand they exist, but I don't find them particularly compelling. And it's not so much the number of changes, rather that it feels unnatural in many circumstances. For example, I'd never have thought to put a comma here:

https://github.com/WordPress/gutenberg/blob/34586fc/blocks/library/embed/index.js#L82

Perhaps it will just take time to adjust. And at least we have ESLint to enforce it if it comes to be overlooked.

@nylen nylen force-pushed the add/eslint-comma-dangle branch from 34586fc to 6671682 Compare May 9, 2017 21:26
@nylen
Copy link
Member Author

nylen commented May 9, 2017

I still think we should do this. Your example doesn't look too bad to me, in fact I think it's pretty natural and also serves to indicate "yes there's more stuff above at this nesting level".

Granted I probably wouldn't be pushing for this change except for the fact that I've already adopted this rule in my preferred coding style personally... though I do find it quite nice to have cleaner diffs when modifying lists of things, like connect properties or component arguments for example.

To update this PR later on (starting with a clean working tree):

git fetch origin && git checkout origin/master -B master
git show add/eslint-comma-dangle .eslintrc.json | git apply
git checkout -B add/eslint-comma-dangle
./node_modules/.bin/eslint . --fix
git commit -am 'Add ESLint `comma-dangle` rule'
git push -f

@nylen nylen force-pushed the add/eslint-comma-dangle branch from 6671682 to 9ef0f1b Compare May 9, 2017 22:18
Copy link
Member

@ntwb ntwb left a comment

Choose a reason for hiding this comment

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

LGTM

@ntwb
Copy link
Member

ntwb commented May 9, 2017

"These are for consistency between the PHP and JavaScript files in the WordPress codebase."

If we're going to do this, may as well do it now, the change no doubt will affect existing PRs and they will need to be updated.

I'm happy to update the changes back to the handbook and add this rule to eslint-config-wordpress today.

Unfortunately I missed the #Core JS chat last night, but I've caught up and read through the chat, if we're going to start updating the JS coding standards, then updating one of those said standards this week in preparation for next weeks chat would be 👌 . I say this as it will give us (rather me) an opportunity to show the workflow and processes of what's required to make a change to the standards.

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

Successfully merging this pull request may close these issues.

3 participants