-
-
Notifications
You must be signed in to change notification settings - Fork 930
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
Tighten ESLint rules. #4462
Tighten ESLint rules. #4462
Conversation
Node.JS v8 is end-of-life in ~31 days on 2019-12-31 Happy to plan for this by ignoring that and dropping Node.js support in the next major release |
19da514
to
64bb726
Compare
Will the next version be a major version one? Regardless, if tests pass on Node.js 8.7.0 (ATM we test the latest 8.x not the minimum supported one), I guess we should be OK. After we decide on the rules, I will make an upstream PR. IMHO we should also move |
ef0a848
to
37dd452
Compare
37dd452
to
57f66fc
Compare
I rebased this. There are still a few errors to fix (any help is welcome) and after we agree on the rules then we can move the newly added rules to our eslint config. IMHO most of these changes are good, but this stuff is always objective :) PS. tests seems to pass on Node.js 8.7.0 so we should be covered regarding 8.x compatibility. |
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.
Most of rules make sense and enforce good practices.
57f66fc
to
17105c5
Compare
17105c5
to
b45bf63
Compare
@hudochenkov if you merge this please don't squash your patches with mine. Feel to squash your patches into one if you prefer, but being you made significant changes, authorship should be preserved. |
I'm afraid all changes will be squashed into one commit, where I will be Co-Author. We don't have any other methods of merge enabled for this repository. |
I believe a rebase and merge is possible and if not, it should be made possible by changing the repository's settings. I don't want to show as the author of changes I didn't make myself. Even if the coauthor line is added, it doesn't show who did what. |
@@ -39,6 +39,6 @@ module.exports.isComment = function isComment(node) { | |||
* @param {Node} node | |||
* @returns {node is (Node & {source: NodeSource})} | |||
*/ | |||
module.exports.hasSource = function isComment(node) { | |||
return !!node.source; | |||
module.exports.hasSource = function hasSource(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.
We could remove the function name here; not sure what's used more in the codebase. This change just ensures consistency.
@hudochenkov I have two more rules I forgot to add, but shouldn't need a lot of changes. Can I push them here? |
Sure, but don't force push :) |
Alright, I'm done with the rule changes. I have an issue opened in eslint-config-stylelint to backport the new rules. |
I open this for discussion. Most of these rules, if not all, should be made to the upstream eslint config.
Notes: