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

Update to eslint 4 and enforce extra rules #1231

Merged
merged 1 commit into from Jun 19, 2017
Merged

Update to eslint 4 and enforce extra rules #1231

merged 1 commit into from Jun 19, 2017

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Jun 16, 2017

No description provided.

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Jun 16, 2017
@xPaw xPaw added this to the 2.3.2 milestone Jun 16, 2017
.eslintrc.yml Outdated
@@ -9,22 +9,28 @@ env:
node: true

rules:
arrow-body-style: 2
arrow-parens: [2, as-needed]
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @thelounge/maintainers

@YaManicKill wants this to be always

Copy link
Member

Choose a reason for hiding this comment

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

I would like this to be always, and here's my reasoning:

We require curly braces around all conditional and loop statements. The reason we do this (and why it's a standard across JS development just now) is because you can try to add a second line to an if, and if you forge tto add in the brackets afterwards, it won't do what you expect (it'll run the second line always).

I feel like we have the same issue with arrow functions, where you can technically do a 1 argument arrow function which doesn't have parenthesis, but if you add in a second argument, and forget to add in the brackets, you will end up with issues. The extra characters don't add much work, and they stop us having issues in the future. Also, it keeps all arrow functions looking consistent.

Copy link
Member

Choose a reason for hiding this comment

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

I personally would prefer as-needed over always, but I won't fight over it :)

I completely get what you're saying, but there is a small subtlety to it though:

if (something)
  doThis();
  doThat();

The above is perfectly valid JS, but probably not what one wants. However:

arr.map(x, y => x * 2); // 💥

is simply broken JS, and a typo that one of our checks (tests, linters, compilers) should detect, regardless of the style preference.

Reason why I'd rather keep as-needed is because of the beauty/simplicty of the new syntax:

arr.map(x => x * 2);

This is much nicer than:

arr.map((x) => x * 2);

And the reason why I won't fight over it is because parentheses are needed in other scenarios:

myFunction(() => doSomething());
myOtherFunction((x, y) => doSomethingElse(x, y));

And also because there is more important in life 😅
Anyway, my recommendation is to keep as-needed and if we start seeing some issues with it, we can become more strict about it.

semi: [2, always]
space-before-blocks: 2
space-before-function-paren: [2, never]
space-in-parens: [2, never]
space-infix-ops: 2
spaced-comment: [2, always]
strict: 2
template-curly-spacing: 2
yoda: 2
Copy link
Member

Choose a reason for hiding this comment

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

Was this removed from recommended or something?

Copy link
Member

Choose a reason for hiding this comment

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

My bad, it was never in there, good addition.

Copy link
Member

@astorije astorije left a comment

Choose a reason for hiding this comment

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

Great stuff!

@xPaw xPaw merged commit 16795cf into master Jun 19, 2017
@xPaw xPaw deleted the eslint4 branch June 19, 2017 17:30
@@ -9,22 +9,28 @@ env:
node: true

rules:
arrow-body-style: 2
arrow-parens: [2, always]
Copy link
Member

Choose a reason for hiding this comment

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

Oh well :(

Copy link
Member

Choose a reason for hiding this comment

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

Yay!

Copy link
Member

Choose a reason for hiding this comment

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

:trollface:

matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
Update to eslint 4 and enforce extra rules
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants