Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
chore: enabled stylistic-type-checked internally#7138
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
chore: enabled stylistic-type-checked internally#7138
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Thanks for the PR,@JoshuaKGoldberg! 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 onhttps://opencollective.com/typescript-eslint. |
netlifybot commentedJun 25, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✅ Deploy Preview fortypescript-eslint ready!
To edit notification comments on pull requests, go to yourNetlify site configuration. |
nx-cloudbot commentedJun 25, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
2a5e9f2 to6718c2fCompare| .join('\n') | ||
| :formatted; | ||
| returncontext.report({ |
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.
which rule suggested this?
TBH I prefer thereturn report style here as it's easier to see that the report is terminal.
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.
no-confusing-void-expression. I'm feeling more and more 👎 on including that one instylistic-type-checked.
| functionisPascalCase(name:string):boolean{ | ||
| return( | ||
| name.length===0|| | ||
| (name[0]===name[0].toUpperCase()&&!name.includes('_')) |
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.
I actually think that the previous form is clearer for this specifica case.
It's also substantially faster (not that it matters at the this speed... but it is twice as fast - ~1.2B ops/s vs ~2.4B ops/s).
I wonder if we should add an option to skip this case from the rule?
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.
Agreed. The main 👎 on this change in my head was that the rule is suggestinglonger code...#7140
| ConditionalExpression:(node):void=>{ | ||
| checkNode(node.test); | ||
| }, |
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.
TBH I personally don't think that multilining arrows is necessarily clearer.
Is there a reason the stylistic set is pushing for this style?
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.
Isn’t thishttps://typescript-eslint.io/rules/no-confusing-void-expression ? You probably already know but posting for other readers :D
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.
Yup it is! +1 to the visibility@rubiesonthesky 😄.
I wonder if it should be moved to thestrict-type-checked config instead...#7130 (comment)
| functiontraverse(node:ts.Node):T|undefined{ | ||
| switch(node.kind){ | ||
| casets.SyntaxKind.ReturnStatement: | ||
| returnvisitor(<ts.ReturnStatement>node); |
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.
| constdefs=(schema.$defs??schema.definitions)as | ||
| |Record<string,JSONSchema4> | ||
| |undefined; |
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.
IIRC I did this intentionally because both of the keys were not techincallyundefined - is that not the case any more?
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.
Yup!no-unnecessary-type-assertion is complaining on them. Bothschema.$defs andschema.definitions areRecord<string, JSONSchema4> | undefined.
| exportinterfaceDependencyConstraint{ | ||
| /** | ||
| * Passing a string for the value is shorthand for a '>=' constraint | ||
| */ | ||
| readonly[packageName:string]:VersionConstraint; | ||
| } |
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.
Making this a record is actually less clear due to the lost comment
| block:TBlock, | ||
| isMethodDefinition:boolean, | ||
| ){ | ||
| constupperScopeAsScopeBase=upperScopeasScope; |
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.
I'm surprised TS allowed this - the reason this cast was there was becauseupperScope is typed asTUpper - and there was a lot of weird behaviour that stemmed from it being a generic instead of the concrete type.
I think there was also some weirdness due to the cyclic nature of the files? I don't fully remmeber though
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('deeplyCopy should convert node correctly',()=>{ | ||
| constast=convertCode('type foo = ?foo<T> | ?(() => void)?'); | ||
| /* eslint-disable @typescript-eslint/dot-notation */ |
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.
why is this disabled?
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.
deeplyCopy is a private member.['deeplyCopy'] bypasses that.
| expect(()=>{ | ||
| parseFile('foo',PROJECT_DIR); | ||
| }).not.toThrow(); | ||
| // bar should throw because it doesn't exist yet | ||
| expect(()=>parseFile(bazSlashBar,PROJECT_DIR)).toThrow(); | ||
| expect(()=>{ | ||
| parseFile(bazSlashBar,PROJECT_DIR); | ||
| }).toThrow(); |
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 the sort of reason I think that the concise arrow function style is better - satisfying this lint rule just added some 160 lines to this file and (IMO) made the codeharder to read
| exportinterfaceGlobalsConfig{ | ||
| [name:string]:GlobalVariableOption; | ||
| } | ||
| exportinterfaceEnvironmentConfig{ | ||
| [name:string]:boolean; | ||
| } |
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.
I believe that these should stay as interfaces to allow for global augmentation from consumers.
| exportinterfaceVisitorKeys{ | ||
| [nodeType:string]:string[]; | ||
| } |
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.
I do like theRecord style more in general, but you can lose some nice implicit documentation sometimes (eg the variable name here).
I'm so torn about using a disable comment for this though because it's so minor.
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 could use comment to say same if this style is kept :)
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.
Yeah this is an interesting discussion. I'll undo the changes fromconsistent-indexed-object-style and file them as a todo to look at later, similar toprefer-nullish-coalescing.
| typeofevaluated.value==='string'&& | ||
| // checks if the string is a character long | ||
| evaluated.value[0]===evaluated.value | ||
| evaluated.value.length===1 |
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Uh oh!
There was an error while loading.Please reload this page.
JoshuaKGoldberg left a comment
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.
(accidentally started a review, but getting pulled out for something - will continue commenting soon)
| .join('\n') | ||
| :formatted; | ||
| returncontext.report({ |
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.
no-confusing-void-expression. I'm feeling more and more 👎 on including that one instylistic-type-checked.
| functionisPascalCase(name:string):boolean{ | ||
| return( | ||
| name.length===0|| | ||
| (name[0]===name[0].toUpperCase()&&!name.includes('_')) |
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.
Agreed. The main 👎 on this change in my head was that the rule is suggestinglonger code...#7140
| ConditionalExpression:(node):void=>{ | ||
| checkNode(node.test); | ||
| }, |
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.
Yup it is! +1 to the visibility@rubiesonthesky 😄.
I wonder if it should be moved to thestrict-type-checked config instead...#7130 (comment)
codecovbot commentedJun 26, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@## v6 #7138 +/- ##======================================= Coverage 87.43% 87.43% ======================================= Files 372 372 Lines 12849 12849 Branches 3813 3813 ======================================= Hits 11235 11235 Misses 1237 1237 Partials 377 377
Flags with carried forward coverage won't be shown.Click here to find out more.
|
Uh oh!
There was an error while loading.Please reload this page.
Co-authored-by: rubiesonthesky <2591240+rubiesonthesky@users.noreply.github.com>
PR Checklist
Overview
Adds
stylistic-type-checkedto the list ofextendsconfigs and removes manual enables of rules contained in it. Most of the changes seem to be fromno-confusing-void-expression, followed byconsistent-indexed-object-style.A couple rules received special treatment:
prefer-nullish-coalescingyet, as there were a lot of complaints from it and I'll look into it separatelyno-empty-functionseemed to be popular for tests, so I moved it to the tests overrides