-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add eslint 7 to CI #1715
Add eslint 7 to CI #1715
Conversation
6 similar comments
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.
Should all the ruleIds become types?
@@ -5,7 +5,7 @@ import { RuleTester } from 'eslint' | |||
const ruleTester = new RuleTester() | |||
, rule = require('rules/no-cycle') | |||
|
|||
const error = message => ({ ruleId: 'no-cycle', message }) |
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.
should this then be test: 'no-cycle'
?
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.
What you mean?
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.
oh sorry, type
. You changed a bunch of ruleID: foo
to be type: foo
- should they all be changed, rather than just deleting them?
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 changed some ruleId: ...
to type: ...
because these types are already correct, like ruleId: "ExportNameSpecifier"
to type: "ExportNameSpecifier"
. In other places they are totally wrong , like ruleId: "no-cycle"
. Filling out the correct type for these cases is too much work for me.
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.
Can these additions made in another PR?
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.
Sure, that's fine.
@benmosher @ljharb Any idea when this one will be released? 🤔 |
Ping @benmosher @ljharb |
@MichaelDeBoey it will be released, like every version, when I have time to do it. There's a number of outstanding PRs i'm hoping to merge first. |
Let me know if and how I could help getting them merged/fixed 🙂 |
The relevant breaking changes in eslint 7 are:
output
are now failed.sourceCode.getComments
is deprecated.