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

tools: disallow mixed spaces and tabs for indents #5135

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Feb 8, 2016

Enable ESLlint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.

While at it, I rearranged the style rules in .eslintrc to be in
alphabetical order. This has two benefits: It means the rules appear
in the same order as they do in the ESLint documentation, easing
cross-referencing. It also means that it is much easier to determine
with visual inspection if a rule is set or not.

@Trott Trott added tools Issues and PRs related to the tools directory. lts-watch-v4.x labels Feb 8, 2016
@rvagg
Copy link
Member

rvagg commented Feb 8, 2016

A bit hard to tell that it's only about that one rule given the change, maybe this would be best as 2 commits? What if we jumped to disallowing tabs completely? I think that would be in line with style expectations anyway.

@mscdex
Copy link
Contributor

mscdex commented Feb 8, 2016

🔥 Down with the tabs! 🔥

@silverwind
Copy link
Contributor

Maybe we should do away with the descriptions above each rule so it's more apparent that these lists are alphabetically sorted. They don't add much value and one can get a better description for each rule by using the links in the file. 😉

@Trott Trott force-pushed the eslint-recommended branch from ee6005f to ce279d7 Compare February 8, 2016 06:37
@Trott
Copy link
Member Author

Trott commented Feb 8, 2016

@rvagg I like your suggestion of splitting this into two commits. Done!

@Trott
Copy link
Member Author

Trott commented Feb 8, 2016

What if we jumped to disallowing tabs completely?

With this new rule added, we would have tabs nearly completely disallowed, but not quite. See eslint/eslint#3521 for the explanation as to why ESLint doesn't support complete tab elimination without an external plugin. /shrug

@Trott
Copy link
Member Author

Trott commented Feb 8, 2016

Maybe we should do away with the descriptions above each rule so it's more apparent that these lists are alphabetically sorted.

Yes, I like that idea. Although we should probably do it for all the lists in the .eslintrc at once rather than just for the Stylistic Issues list that is modified in this PR...

@targos
Copy link
Member

targos commented Feb 8, 2016

The test changes are in the wrong commit

Trott added 2 commits February 8, 2016 10:42
Rearrange the style rules in .eslintrc to be in alphabetical order.

This has two benefits:

It means the rules appear in the same order as they do in the ESLint
documentation, easing cross-referencing.

It also means that it is much easier to determine with visual inspection
if a rule is set or not.
Enable eslint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.
@Trott Trott force-pushed the eslint-recommended branch from ce279d7 to d7dca68 Compare February 8, 2016 18:42
@Trott
Copy link
Member Author

Trott commented Feb 8, 2016

Thanks, @targos! Fixed and force-pushed.

@targos
Copy link
Member

targos commented Feb 8, 2016

LGTM if CI is happy: https://ci.nodejs.org/job/node-test-pull-request/1588/

@Trott
Copy link
Member Author

Trott commented Feb 8, 2016

CI failure looks like it's actually due to #5109. We can re-run once that gets tidied up.

@Trott
Copy link
Member Author

Trott commented Feb 8, 2016

CI is back in decent shape, so let's try this again: https://ci.nodejs.org/job/node-test-pull-request/1593/

@silverwind
Copy link
Contributor

LGTM. Want to follow up with another PR to remove the comments?

Trott added a commit that referenced this pull request Feb 9, 2016
Rearrange the style rules in .eslintrc to be in alphabetical order.

This has two benefits:

It means the rules appear in the same order as they do in the ESLint
documentation, easing cross-referencing.

It also means that it is much easier to determine with visual inspection
if a rule is set or not.

#5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Trott added a commit that referenced this pull request Feb 9, 2016
Enable eslint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.

PR-URL: #5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott
Copy link
Member Author

Trott commented Feb 9, 2016

Landed in a84bf2c and eea987f

@Trott Trott closed this Feb 9, 2016
rvagg pushed a commit that referenced this pull request Feb 10, 2016
Rearrange the style rules in .eslintrc to be in alphabetical order.

This has two benefits:

It means the rules appear in the same order as they do in the ESLint
documentation, easing cross-referencing.

It also means that it is much easier to determine with visual inspection
if a rule is set or not.

#5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg pushed a commit that referenced this pull request Feb 10, 2016
Enable eslint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.

PR-URL: #5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Rearrange the style rules in .eslintrc to be in alphabetical order.

This has two benefits:

It means the rules appear in the same order as they do in the ESLint
documentation, easing cross-referencing.

It also means that it is much easier to determine with visual inspection
if a rule is set or not.

#5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Enable eslint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.

PR-URL: #5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Rearrange the style rules in .eslintrc to be in alphabetical order.

This has two benefits:

It means the rules appear in the same order as they do in the ESLint
documentation, easing cross-referencing.

It also means that it is much easier to determine with visual inspection
if a rule is set or not.

#5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2016
Enable eslint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.

PR-URL: #5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@MylesBorins MylesBorins mentioned this pull request Feb 18, 2016
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Rearrange the style rules in .eslintrc to be in alphabetical order.

This has two benefits:

It means the rules appear in the same order as they do in the ESLint
documentation, easing cross-referencing.

It also means that it is much easier to determine with visual inspection
if a rule is set or not.

#5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
Enable eslint rule disallowing mixing tabs and spaces for indentation.
Modify the one file that had been mixing tabs and spaces.

PR-URL: #5135
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
@Trott Trott deleted the eslint-recommended branch January 9, 2022 22:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants