-
Notifications
You must be signed in to change notification settings - Fork 13
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
Use ES6 syntax features #82
Conversation
lib/rules/no-inner-compare.js
Outdated
ExpressionStatement: function(node) { | ||
var expression = node.expression; | ||
ExpressionStatement(node) { | ||
const {expression} = node; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to use let
instead of const
unless we're talking about actual singleton constants (those at the top-level module scope that already use uppercase names) or imports
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that.
But in case you haven't come across this reasoning, I find that const
, as the ESLint prefer-const
rule states, can help with "reducing cognitive load and improving maintainability.", since it makes clearer at a glance what items are counters and such (with let
) and which are simply new scoped variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I know about that reasoning, and I don't buy it... 😅
the author of the let/const spec explicitly stated that this was not the reason, and yet people use it that way 🤷♂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think specs determine what we ought to or have to follow, but we don't have to agree even with the spec creators on why/how we have to follow them. But ok, sure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also by imports using const
do you mean require
statements?
.eslintrc
Outdated
@@ -1,6 +1,7 @@ | |||
{ | |||
"extends": "eslint:recommended", | |||
"env": { | |||
"es6": true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are we using any ES6 APIs that require this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need at least "parserOptions": { "ecmaVersion": 6 }
to cover the ES6 syntax such as const
and shorthand methods.
es6: true
includes this syntax automatically, though it also allows ES6 globals which the former does not. I can change to the former if you prefer, but just seems easier in case we need the globals later since we're already on ES6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, I think I'd prefer the parserOptions
approach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And rebased.
4a74e18
to
014a849
Compare
…ons, template literals, shorthand properties and methods, spread, includes) - Refactoring: Consistent quotes (single)
…cap constants; consistent semi-colons
const
, destructuring, arrow functions, template literals, shorthand properties and methods, spread, includes)