Conversation
|
I'm not sure why I had a pending change to update to 3.0. It doesn't look like I released it. I'm going to leave it in, and make this PR be the bases for 3.1, hoping for the best. |
|
@Mingun do you have any opinions about the things I changed? I'm sorry I didn't do the patch in two steps to make it easier to separate the moves from the changes. |
index.js
Outdated
| "no-self-assign": "error", | ||
| "no-self-compare": "error", | ||
| "no-sequences": "error", | ||
| "no-shadow": "error", |
There was a problem hiding this comment.
This is going to cause good changes in Peggy.
There was a problem hiding this comment.
I'm not sure... It creates a lot of strange renames (when function parameter shadows external variable) without much benefit, IMO. But I'm not strongly against it. Maybe enable shadowing for some names (node, context)?
There was a problem hiding this comment.
I'll turn it off for now. Perhaps we can enable it some other time.
|
|
||
| // Disabled because multiple spaces are often used for alignment. | ||
| "no-multi-spaces": "off", | ||
| "no-mixed-operators": "error", |
There was a problem hiding this comment.
This is going to cause a few sets of parens, which is worth it, imo.
|
|
||
| // Disabled because `eval` has legitimate uses. | ||
| "no-eval": "off", | ||
| "no-eval": "error", |
There was a problem hiding this comment.
We'll put an eslint-ignore in the two places where we want eval explicitly.
| "no-constructor-return": "error", | ||
| "no-constant-binary-expression": "error", | ||
| "no-div-regex": "off", | ||
| "no-confusing-arrow": "error", |
There was a problem hiding this comment.
There are a few places that are legitimately confusing. We'll fix those.
| @@ -87,6 +89,7 @@ module.exports = { | |||
| "@typescript-eslint/func-call-spacing": "error", | |||
| "@typescript-eslint/indent": ["off", 2], // Broken, see https://github.com/typescript-eslint/typescript-eslint/issues/1824 | |||
| "@typescript-eslint/init-declarations": "error", | |||
There was a problem hiding this comment.
This is going to be a biggish change, but I think it's good for quality.
| "eqeqeq": "error", | ||
| "func-name-matching": "error", | ||
| "func-names": "off", // Tedious | ||
| "func-style": ["error", "declaration"], |
There was a problem hiding this comment.
This only causes one change in tests.
| ignorePattern: "c8", | ||
| ignoreConsecutiveComments: true, | ||
| }], | ||
| "class-methods-use-this": "error", |
There was a problem hiding this comment.
Causes a few things that will have to be ignored (abstract methods) and a few places that we might want to move to static.
| "no-unused-vars": "error", | ||
|
|
||
| // This is going to cause problems, but I don't care. | ||
| "no-use-before-define": "error", |
There was a problem hiding this comment.
This causes a pretty large set of changes, most of which I really like.
|
It would be easier to me see the changes if you show what changes it led to in peggy (making PR there). |
|
I didn't mind having to explicitly opt out when I was using practices that were questionable. |
index.js
Outdated
| "no-self-assign": "error", | ||
| "no-self-compare": "error", | ||
| "no-sequences": "error", | ||
| "no-shadow": "error", |
There was a problem hiding this comment.
I'm not sure... It creates a lot of strange renames (when function parameter shadows external variable) without much benefit, IMO. But I'm not strongly against it. Maybe enable shadowing for some names (node, context)?
2 participants
Update dependencies. Update rules. This will cause a bunch of things to lint-fail in peggy, but it's time for that.