-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): add consistent-type-definitions
rule
#463
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): add consistent-type-definitions
rule
#463
Conversation
prefer-type-alias
rule
Codecov Report
@@ Coverage Diff @@
## master #463 +/- ##
==========================================
- Coverage 94.25% 94.22% -0.03%
==========================================
Files 107 108 +1
Lines 4388 4419 +31
Branches 1208 1217 +9
==========================================
+ Hits 4136 4164 +28
Misses 147 147
- Partials 105 108 +3
|
Hey @otofu-square - thanks for doing this! Could you please "merge" the two rules (your new One goal of eslint is to prevent the ability to get yourself into inconsistent states via config. You won't be able to delete {
"deprecated": true,
"replacedBy": "consistent-type-definitions"
} |
@bradzacher
I see. |
@bradzacher ...
schema: [
{
type: 'object',
properties: {
preferDeclaration: {
enum: [
'type',
'interface',
],
},
}
}
] |
In this case, I would say that generally the preference for rule config is to have the first argument be a string, followed by an object for additional config items. I.e. As a tuple type it'd be type Options = [
('type' | 'interface')?,
{
opt1 ?: value,
}?,
]; |
1c7059c
to
02c1e55
Compare
prefer-type-alias
ruleconsistent-type-definitions
rule
c6e7838
to
a2c96b2
Compare
8342a2c
to
5fe1a53
Compare
@bradzacher |
consistent-type-definitions
ruleconsistent-type-definitions
rule
5fe1a53
to
0427c58
Compare
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.
you should avoid force pushing when possible.
if you force push, then github cannot track your history, as your force push obliterates it!
don't forget to import your rule into index.ts
in the root of the plugin.
the rule code looks good to me!
Just need to create a readme for the rule
packages/eslint-plugin/src/rules/consistent-type-definisions.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/src/rules/consistent-type-definisions.ts
Outdated
Show resolved
Hide resolved
Thanks for your review.
It makes sense to me!
Oops, got it. |
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.
it all LGTM
@bradzacher |
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.
Docs mostly look good.
Would suggest two things:
- move your "Options" section so that it is before "Rule Details"
- You could shorten the section so it's a bit clearer:
### Options
This rule accepts one string option:
- `"interface"`: enforce using `interface`s for object type definitions.
- `"type"`: enforce using `type`s for object type definitions.
For example:
```CJSON
{
// Use type for object definitions
"@typescript-eslint/consistent-type-definitions": ["error", "type"]
}
```
Oh, I really love to have this rule come alive!!! 👍 Thank you for this great work!!! |
@otofu-square - there are two merge conflicts which need resolving (github thinks they're too "complex", so I can't do them for you via the github web UI). Once they're resolved I will merge to master. |
@bradzacher |
@otofu-square - that looks like a bug in the documentation checker! I'll look into it tomorrow morning. |
ref #142
Addsprefer-type-alias
rule which is the inverse ofprefer-interface
Adds
consistent-type-definitions
rule to consist type definitionsinterface
ortype
.