feat(eslint-plugin): add no-dynamic-delete rule #1058
Conversation
|
Thanks for the PR, @dvorsky! 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 on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
Codecov Report
@@ Coverage Diff @@
## master #1058 +/- ##
==========================================
+ Coverage 94.14% 94.15% +<.01%
==========================================
Files 115 116 +1
Lines 5023 5031 +8
Branches 1406 1408 +2
==========================================
+ Hits 4729 4737 +8
Misses 166 166
Partials 128 128
|
There was a problem hiding this comment.
thanks for your contribution!
A tip for future - the issues are usually a better place to find things that need contributing to. That way you can find rules / options that other people have requested.
Happy to accept this PR and get closer to parity with tslint though.
I see you copied the test cases from the tslint repo. These form a good basis, but they mostly only test the positive case (i.e. the case where the rule matches).
One thing we look for in tests is coverage for the negative cases as well, to ensure there are no unexpected crashes, or false positives.
Could you please add some test cases like
delete container[variable];
delete container[++i];
delete container[i--];
delete container;
delete container.fn();
delete 'a';
delete 1;
delete container!;
delete (container as any).a;Whilst some of these are semantically invalid code (typescript will report an error for them), they are syntactically valid, which means we have to ensure the rule doesn't crash on them.
| return; | ||
| } | ||
|
|
||
| if ((node.argument as TSESTree.MemberExpression).computed) { |
There was a problem hiding this comment.
prefer a physical check to a type cast, as it increases safety
| if ((node.argument as TSESTree.MemberExpression).computed) { | |
| if (node.argument.type === TSESTree.MemberExpression && node.argument.computed) { |
| }, | ||
| schema: [], | ||
| }, | ||
| defaultOptions: [null], |
There was a problem hiding this comment.
| defaultOptions: [null], | |
| defaultOptions: [], |
|
I completely forgot that there was already a PR for this: #565. |
Hi! I wanted to contribute to the project so I looked at roadmap and saw that no-dynamic-delete rule was unimplemented. There was no issue for it so I'm not sure if you're open to merging this, but wanted to try my hand at creating eslint rules anyway 😄