Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
fix(eslint-plugin): [no-empty-interface] disable autofix for declaration merging with class#5920
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nx-cloudbot commentedNov 2, 2022 • 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.
Thanks for the PR,@yeonjuan! 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 commentedNov 2, 2022 • 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 settings. |
codecovbot commentedNov 2, 2022 • 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 @@## main #5920 +/- ##======================================= Coverage 91.27% 91.28% ======================================= Files 366 366 Lines 12348 12352 +4 Branches 3610 3612 +2 =======================================+ Hits 11271 11275 +4 Misses 768 768 Partials 309 309
Flags with carried forward coverage won't be shown.Click here to find out more.
|
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.
Great stuff, thanks for sending this in@yeonjuan! 🎊
Mostly requesting changes on getting more test coverage. Would be nice to see a bit of refactoring to reduce the lines of code too.
| // Check if interface is within ambient declaration | ||
| letuseAutoFix=true; | ||
| lethasSuggest=true; |
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: I found it confusing how the previous variable starts withuse but this starts withhas, even though they're pretty much the same thing. Can we make the names more similar?
| lethasSuggest=true; | |
| letuseSuggestion=true; |
Note: after applying the refactor I suggest later in the file, the variable might be unnecessary.
| if(util.isDefinitionFile(filename)){ | ||
| constscope=context.getScope(); | ||
| if(scope.type==='tsModule'&&scope.block.declare){ |
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] Now that theconst scope = ... line is earlier, these twoif statements can be combined:
if(util.isDefinitionFile(filename)&&scope.type==="tsModule"&&scope.block.declare){useAutoFix=false;}
...so the code can be simplified a bit more, to giveuseAutoFix an initial value:
letuseAutoFix= ...;
I would even suggest making a separate variable for whether it's merged with a class declaration:
constmergedWithClassDeclaration=scope.set.get(node.id.name)?.defs?.some((def)=>def.node.type===AST_NODE_TYPES.ClassDeclaration);constuseAutoFix=!mergedWithClassDeclaration&& ...;
Thoughts?
Uh oh!
There was an error while loading.Please reload this page.
yeonjuan commentedNov 18, 2022
@JoshuaKGoldberg Thanks for the review!. I fixed it and add some test cases |
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.
Fantastic, thanks for iterating on this with me@yeonjuan! It looks great ✨
…ion merging with class (typescript-eslint#5920)* fix(eslint-plugin): [no-empty-interface] disable autofix for declaration merging with class* fix comment* fix lint* Apply review
PR Checklist
Overview