-
-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
fix(eslint-plugin): [ban-types] add option extendDefaults #1379
Conversation
Thanks for the PR, @threehams! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
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.
Thanks - this LGTM as it currently stands.
RC for this question - one thing I might ask is a new option.
type Options = [
{
types?: Types;
extendDefaults?: boolean;
},
]
Which would do the following: If false or omitted, it would work as you have just implemented - all or nothing. If true, it would do Object.assign(defaultOpts, userOpts.types)
(i.e. extends and overrides the defaults with the user options).
This would save people copy-pasting the defaults all of the time, which would make it easier for them to include any future changes to defaults we do.
WDYT?
We might just be dancing around the issue of the The official recommendation is to use If the message were more generic and it didn't auto-fix, then this seems like a good use for the suggestions API, though it would require ESLint 6.7. |
This is related to #848, where the fixer for |
In that case, I'll add extendDefaults here since it'll be taken care of in that PR. This change could take a little longer, it's not as clear how to do it without adding more one-off code in here. Maybe necessary though. |
I think this is definitely a one-off use case. This rule is an edge case. I'm happy with the one-off code living in this rule. Thinking about it, if you add |
I think this would be the best option. |
Updated code, tests, docs. Edited description as this is no longer a breaking 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.
LGTM - thanks for seeing this through.
Codecov Report
@@ Coverage Diff @@
## master #1379 +/- ##
=======================================
Coverage 95.53% 95.53%
=======================================
Files 142 142
Lines 6625 6630 +5
Branches 1897 1900 +3
=======================================
+ Hits 6329 6334 +5
Misses 106 106
Partials 190 190 |
Special-case
ban-types
rule to replace default options instead of merging ifextendDefaults
is set tofalse
.Fixes #686.