-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,19 +72,24 @@ export default util.createRule<Options, MessageIds>({ | |
}, | ||
defaultOptions: [ | ||
{ | ||
allowAny: true, | ||
allowBoolean: true, | ||
allowNullish: true, | ||
allowNumberAndString: true, | ||
allowRegExp: true, | ||
Comment on lines
+75
to
+79
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @JoshuaKGoldberg i don't remember our discussion here - why did we turn all of the options on? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The issue was that it was chafing a lot of users by being very strict by default. A lot of folks are intentionally e.g. logging template literal strings with various primitives inside. This does bring up the idea of having different configs in the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your example is template strings - but this is restrict-plus-operands. So was this an accidental mix-up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it applies to both of them. I've seen both happen with users. Which is why I keep mixing them up in comments 😅 to me, they're similar -just not exactly the same- cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the question is - why even have these turned on at all if all the options are on by default? Eg for RPO there's the following cases. Of these 16 cases the default options mean the rule only matches FOUR cases - two of which are TS already errors. So we're recommending a rule to everyone to stop them from doing: tl;dr - why are we even turning on the rules in the recommended set if they don't do anything with an overly permissive default option? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A couple reasons swayed me:
From a performance perspective, if one of these two rules is already asking for the types of these, I some level of caching on the TypeScript side would make the other rule not a significant change. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thing I'd point to is that we added this big caution block to the RPO docs because we didn't think people using the options was good - and yet it's the default that everyone uses these options? Just seems like we're contradicting ourselves "don't use these options but also they're all turned on by default" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah :/ agreed. I don't like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Personal preference: |
||
skipCompoundAssignments: false, | ||
}, | ||
], | ||
create( | ||
context, | ||
[ | ||
{ | ||
skipCompoundAssignments, | ||
allowAny, | ||
allowBoolean, | ||
allowNullish, | ||
allowNumberAndString, | ||
allowRegExp, | ||
skipCompoundAssignments, | ||
}, | ||
], | ||
) { | ||
|
feat(eslint-plugin): apply final v6 changes to configs #7110
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
feat(eslint-plugin): apply final v6 changes to configs #7110
Changes from all commits
eb03fb4
632dee0
6e27cfc
d47cc0e
97f8815
380461d
e8855fa
File filter
Filter by extension
Conversations
Jump to