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
Update: Add enforceForClassMembers option to no-useless-computed-key #12110
Update: Add enforceForClassMembers option to no-useless-computed-key #12110
Conversation
09e0065
to
9cfc1a3
Compare
Marking as accepted, as the issue is accepted. |
A couple of edge cases.
I'm not sure what the spec says about this, how different engines interpret these cases, and what should the rule do. Could be safer to leave this as a possible false negative?
I think that this should be handled (not reported), because it changes behavior. |
Thank you for comments!
Looks like Chrome, Firefox and EDGE all interpret it to be the same, so I don't see much reason not to report it. Though it's handled differently in TypeScript, I'll open an issue about it. |
static So, at the moment, only these two are certainly not 'useless' computed keys:
|
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 for the delay!
The name can be enforceForClassMembers
, like the recently added options in the accessor-pairs and computed-property-spacing rules.
The new option also has to be documented in no-useless-computed-key.md, so the users can be aware of it.
I left some notes regarding the tests, the rule itself already looks good!
3f50a5f
to
90158b0
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.
Thanks for the changes! I left a couple of notes.
I forgot this - could you please update the PR title with the new option's name |
b30843b
to
bd117a4
Compare
@mdjermanovic Sorry for the delay and thanks for your comments |
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.
LGTM, thanks!
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.
LGTM, thanks!
@aladdin-add, we took a look at this in the TSC meeting today and noticed that you had self-requested a review. Is this something you still want to look at? It has three other approvals, so we can merge this as part of the next release unless you have any objections. |
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 don't fully understand the special cases for static "constructor"
and "prototype"
, but this change looks good to me. Thanks for contributing!
please feel free to merge it, since I don't have a time to make a review atm. sorry for the inconvenience! |
Thanks for contributing! |
What is the purpose of this pull request? (put an "X" next to item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #12041.
What changes did you make? (Give an overview)
enforceForClassMembers
option to the schemaProperty
visitor to acheck
functionMethodDefinition
visitor that ischeck
if option is enabled and noop function if it's not (would it be better to check node type incheck
function?)['constructor']
method name (like__proto__
in object, it has different behavior)Is there anything you'd like reviewers to focus on?
I'm not sure aboutcheckMethods
option name. What is the rule oncheck
in option names? Ismethods
clear enough considering that it also handles accessors (even though they share same node kind it might be not so obvious to users). Would it require a new option once class fields would be standardized (if no, it could beclassMembers
)?