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
Respect the original TSLint configuration of ban-types
#353
Respect the original TSLint configuration of ban-types
#353
Conversation
In the past, this tool didn't respect the TSLint configuration of `ban-types`, it means even if TSLint specified `ban-types` with the argument, generated ESLint configuration doesn't contain the detailed configuration of that. For example, when the TSLint configuration is like the following: ``` { "rules": { "ban-types": [true, ["Object", "Use {} instead."], ] } } ``` then, older tool generates the following ESLint configuration: ``` "rules": { "@typescript-eslint/ban-types": "error" } ``` According to the official documentation, probably this generated result doesn't make sense because each rule requires the type name (and the message). https://github.com/typescript-eslint/typescript-eslint/blob/f3160b471f8247e157555b6cf5b40a1f6ccdc233/packages/eslint-plugin/docs/rules/ban-types.md So this commit makes the generated ESLint configuration to be suitable to the certainly working configuration, like so: ``` "rules": { "@typescript-eslint/ban-types": [ "error", { "types": { "Object": { "message": "Use {} instead." } } } ] } ``` See also: https://palantir.github.io/tslint/rules/ban-types/ Related ticket: typescript-eslint#352
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.
This looks great so far, thanks @moznion! I left a few (nit)picks in the source, but if you disagree with them I don't think they should block this PR from being merged.
One thing that is blocking it is the lack of test coverage for two cases:
- Line 12: testing what happens if the input argument isn't an array
- Line 17: testing what happens if the input array doesn't have a type
src/rules/converters/ban-types.ts
Outdated
message: string; | ||
}; | ||
|
||
const bannedTypesObj: { [key: string]: ConvertBanTypeArgument | null } = {}; |
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.
Small nit, Record
is now the built-in type that can handle these kinds of types:
const bannedTypesObj: { [key: string]: ConvertBanTypeArgument | null } = {}; | |
const bannedTypesObj: Record<string, ConvertBanTypeArgument | null> = {}; |
src/rules/converters/ban-types.ts
Outdated
message: string; | ||
}; | ||
|
||
const bannedTypesObj: { [key: string]: ConvertBanTypeArgument | null } = {}; |
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.
A few naming nits for this file:
- "
Obj
" is unnecessary inbannedTypesObj
; could you rename tobannedTypes
? typ
isn't very clear and I was confused at first as to what it means; could you rename tobannedType
or similar?
src/rules/converters/ban-types.ts
Outdated
break; | ||
} | ||
|
||
const typ = rule[0]; |
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.
Nit: if you want to be really slick ✨ 😉 you could do something like:
const [bannedType, message] = rule;
...so you can turn the object assigned to bannedTypesObj[typ]
into { message }
.
Oh, and feel free to push new commits instead of force pushing. Helps keep the review comments findable 👍 |
Thanks for your lightning-fast reviews! I'll tweak them! |
@JoshuaKGoldberg I've just pushed tweaked commits. Could you please review them again? |
Oops, somehow I removed logic that checks a rule is array object or not. I added that at 3d8ded3. |
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 @moznion! |
Thank you for your support, @JoshuaKGoldberg! |
PR Checklist
ban-types
converter should respect the original TSLint configuration for each type descriptor #352status: accepting prs
Overview
When it gives the following TSLint configuration:
as-is:
maybe this doesn't make sense.
to-be:
This pull-request enhances a converter for ban-types to make the behaviour to be expected: https://github.com/typescript-eslint/typescript-eslint/blob/f3160b471f8247e157555b6cf5b40a1f6ccdc233/packages/eslint-plugin/docs/rules/ban-types.md