-
-
Notifications
You must be signed in to change notification settings - Fork 223
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
fix(eslint-plugin): [no-empty-lifecycle-method] incorrect suggestions and correct reports #606
Conversation
Nx Cloud ReportCI ran the following commands for commit 1399bb9. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch Sent with 💌 from NxCloud. |
67334c1
to
57ef860
Compare
Codecov Report
@@ Coverage Diff @@
## master #606 +/- ##
==========================================
- Coverage 85.76% 85.75% -0.02%
==========================================
Files 89 89
Lines 2424 2443 +19
Branches 421 430 +9
==========================================
+ Hits 2079 2095 +16
Misses 205 205
- Partials 140 143 +3
|
911edf4
to
d959f66
Compare
daa28f6
to
328968c
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.
Sorry about the conflicts from when the doc generation work went in
@@ -67,12 +63,6 @@ export default createESLintRule<Options, MessageIds>({ | |||
interfaceName, | |||
fixer, | |||
), | |||
getImportRemoveFix( |
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 proposing to remove it because we can't guarantee that the import is only being used for this class, in other words it might be being used in another class(es) also in the same file and so we could probably be suggesting a code that does not compile. Does that make sense for you, @JamesHenry?
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.
Can we do a simple string-based check on the file contents for that?
If there is more than one occurrence of the string, we leave it (yes, this could have false positives, e.g. from comments, but still better than never removing it for the common case), if there is only one occurrence (the import) then we remove it?
WDYT?
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.
Indeed! I've submitted this check, PTAL!
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.
See my comment response and there are conflicts from one of your earlier PRs being merged
… and correct reports
In addittion to the fix on suggestion, it now reports literal
MethodDefinition
s and removes interfaces withinMemberExpression
s.Fixes #604.