Navigation Menu

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

Enable comma-dangle rule in ESLint, fix all errors. #3324

Merged
merged 1 commit into from Sep 20, 2017

Conversation

kdzwinel
Copy link
Collaborator

Closes #3304

@@ -38,11 +38,17 @@ module.exports = {
"strict": [2, "global"],
"prefer-const": 2,
"curly": [2, "multi-line"],
"comma-dangle": [2, {
"arrays": "always-multiline",
"objects": "always-multiline",
Copy link
Collaborator Author

@kdzwinel kdzwinel Sep 13, 2017

Choose a reason for hiding this comment

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

"always-multiline" means that trailing comma is required only if object/array spans to multiple lines

"objects": "always-multiline",
"imports": "never",
"exports": "never",
"functions": "never"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

trailing commas in imports/exports/functions aren't used in lighthouse code, so I turned them off. (function(a,b,)?? Yuck)

Copy link
Collaborator

Choose a reason for hiding this comment

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

it's actually pretty nice for clean diffs and easy copy paste of multi-line function arguments :) (need to wait awhile for node support to get there though)

SomethingVeryLong.SomethingVeryLong = function somethingVeryLong(
  aLongName,
  aSecondLongName,
) {
 //
};

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, with "always-multiline" it would make sense 👍

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

LGTM, though others may want eyes on.

All the places commas were added seem reasonable, even if it gets a bit ridiculous with some of those

        },
      },
    },
  ],
};

:)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

the conflicts will be fun, but it's worth it. :)

@brendankenny brendankenny merged commit c780dfa into GoogleChrome:master Sep 20, 2017
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.

None yet

4 participants