-
Notifications
You must be signed in to change notification settings - Fork 1
feat: add suggestion to remove parameter properties #14
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
base: main
Are you sure you want to change the base?
feat: add suggestion to remove parameter properties #14
Conversation
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 good start! This one will be a bit more involved though. 💪
You can ignore the Compliance and Lint Docs failures, by the way. I'm taking a look now... 🤔 |
OK if you merge the latest As for Compliance, it's mtfoley/pr-compliance-action#401. I fixed it in this PR by switching the fix link from As for the odd Compliance errors,
|
fa85c1b
to
6c9db62
Compare
6c9db62
to
03e62b2
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14 +/- ##
===========================================
- Coverage 100.00% 96.24% -3.76%
===========================================
Files 6 6
Lines 174 266 +92
Branches 18 32 +14
===========================================
+ Hits 174 256 +82
- Misses 0 10 +10 ☔ View full report in Codecov by Sentry. |
03e62b2
to
0eae0e1
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.
Hello @JoshuaKGoldberg, I have a version that works, but it has some details. Could you please take a look?
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 progress! 🚀
const constructor = node.parent; | ||
if (constructor.type !== AST_NODE_TYPES.FunctionExpression) { | ||
return; | ||
} |
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.
These three if
statements are still missing coverage. You can check this locally with pnpm run test run --coverage
-> opening coverage/index.html
in a browser. For each case, >=1 of the following 3 is happening:
- It's a good check for a real case that should be unit tested
- The case it checks for can't be hit, and:
- Removing it can be done without issue
- It's helpful for type narrowing - which would mean a type assertion should be used here, with a comment to a new or existing issue on typescript-eslint to improve the AST
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.
[Testing] These tests are looking good! I took a deeper look through code and think there are some more cases to handle:
- Parameter property with both an accessibility and
readonly
- Comments before, inside, and after the parameter properties
- Multiple parameters - both as properties and just normal ones
- Existing properties that happen to have the same name
- Constructor bodies with statements inside nodes inside the body e.g.:
I'm not suggesting changing logic to handle deep constructor bodies better, I think it's fine as-is. This is just something to future-proof.
constructor(...) { { this.a = a; } }
Apologies for not mentioning these sooner, seeing the implementation was very helpful for me to ideate how to break the suggestions.
node.expression.left.object.type === | ||
AST_NODE_TYPES.ThisExpression && | ||
node.expression.left.property.type === AST_NODE_TYPES.Identifier && | ||
node.expression.left.property.name === paramName, |
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.
[Testing] Similar comment here as earlier in this file, several of these lines can be commented out without any resultant test failures.
fix(fixer) { | ||
const fixes: RuleFix[] = []; | ||
|
||
fixes.push( | ||
fixer.insertTextBefore( | ||
classBody.body[0], | ||
`${fieldDeclaration}\n`, | ||
), | ||
); |
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.
[Style] A bit more succinct of a declaration: fixer functions can be generators! Instead of returning a full array of fixes, they can yield back each fix as they get to them. Switching over saves something like 10 lines in total 😎:
fix(fixer) { | |
const fixes: RuleFix[] = []; | |
fixes.push( | |
fixer.insertTextBefore( | |
classBody.body[0], | |
`${fieldDeclaration}\n`, | |
), | |
); | |
*fix(fixer) { | |
yield fixer.insertTextBefore( | |
classBody.body[0], | |
`${fieldDeclaration}\n`, | |
); |
👋 @AlexMunoz, just checking in - are you blocked on anything? |
PR Checklist
status: accepting prs
Overview