-
Notifications
You must be signed in to change notification settings - Fork 100
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
Add SonarTS converters. #1085
Add SonarTS converters. #1085
Conversation
Wowee, all of the rules in a single go: this is very impressive @Res42. Normally I would request they all be split up into separate PRs but since you've already gone through the effort, and most of these seem pretty straightforward, it seems reasonable to review them all here. 👍
Distinct business logic. 👍 Each input TSLint rule is associated with a single converter, regardless of what ESLint rule(s) any converters may output. Other converters might result in the same output rule (which is what you're seeing and asking about, I think), but that doesn't impact each other converter. [Architecture]
Any time two or more rule converters output the same ESLint rule. You've noted quite a few places where, as of this PR, that might happen: e.g. both
Right now a merger will be called in
|
src/converters/lintConfigs/rules/ruleConverters/eslint-plugin-sonarjs/use-primitive-type.ts
Show resolved
Hide resolved
That is a bug! Were there any other rules you noticed? Go ahead and file an issue if you'd like, much appreciated. I can go through and check them after this to see as well. |
|
I saw this issue: #1087 but I have one more problem with the
The current default options: Default Optionsconst defaultTypes = {
String: {
message: 'Use string instead',
fixWith: 'string',
},
Boolean: {
message: 'Use boolean instead',
fixWith: 'boolean',
},
Number: {
message: 'Use number instead',
fixWith: 'number',
},
Symbol: {
message: 'Use symbol instead',
fixWith: 'symbol',
},
Function: {
message: [
'The `Function` type accepts any function-like value.',
'It provides no type safety when calling the function, which can be a common source of bugs.',
'It also accepts things like class declarations, which will throw at runtime as they will not be called with `new`.',
'If you are expecting the function to accept certain arguments, you should explicitly define the function shape.',
].join('\n'),
},
// object typing
Object: {
message: [
'The `Object` type actually means "any non-nullish value", so it is marginally better than `unknown`.',
'- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.',
'- If you want a type meaning "any value", you probably want `unknown` instead.',
].join('\n'),
},
'{}': {
message: [
'`{}` actually means "any non-nullish value".',
'- If you want a type meaning "any object", you probably want `Record<string, unknown>` instead.',
'- If you want a type meaning "any value", you probably want `unknown` instead.',
].join('\n'),
},
object: {
message: [
'The `object` type is currently hard to use ([see this issue](https://github.com/microsoft/TypeScript/issues/21732)).',
'Consider using `Record<string, unknown>` instead, as it allows you to more easily inspect and use the keys.',
].join('\n'),
},
}; |
Yes - more generally, whenever there's a known introduction the need for a merger, the same PR should include that merger. This way we don't introduce error cases.
Cool 😄 . Are you up for posting in #1087 then? |
Merger check according the new handling described in #1086;
|
src/converters/lintConfigs/rules/ruleConverters/eslint-plugin-sonarjs/no-duplicate-string.ts
Show resolved
Hide resolved
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 is beautiful... just one little thing off!
src/converters/lintConfigs/rules/ruleConverters/eslint-plugin-sonarjs/no-return-type-any.ts
Outdated
Show resolved
Hide resolved
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.
Grr, wrong button*
FYI @KingDarBoja - I'm not explicitly requesting your review because this PR is huge and I don't want to bug you for things I'm going over anyway 😄 , but if you request a review from yourself I'll hold off merging until you've approved. |
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.
Fantastic, thanks so much @Res42!
PR Checklist
status: accepting prs
Overview
I added the rules from the table.
I have some questions:
Also in the new converter
convertUsePrimitiveType
I use the ESLint rule@typescript-eslint/ban-types
with the typesString
,Boolean
, andNumber
. Should I do this? I don't know if it is better if I just use the default options (by not giving any arguments) or if the merger can handle if this converter uses these explicit types and another converter use the default (no args) options.@typescript-eslint/no-use-before-define
existing converter (convertNoUseBeforeDeclare
). This converter does not turn off the default ESLint ruleno-use-before-define
. But in the @typescript-eslint/no-use-before-define documentation it is written that// note you must disable the base rule as it can report incorrect errors
. Is this a bug in the existing converter or should I not turn off theno-use-before-define
rule in my converter?