Skip to content
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

Merged

Conversation

ark120202
Copy link
Contributor

@ark120202 ark120202 commented Aug 18, 2019

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)

  • Added a new enforceForClassMembers option to the schema
  • Extracted Property visitor to a check function
  • Added a MethodDefinition visitor that is check if option is enabled and noop function if it's not (would it be better to check node type in check function?)
  • Changed visitor code to allow ['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 about checkMethods option name. What is the rule on check in option names? Is methods 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 be classMembers)?

@jsf-clabot
Copy link

jsf-clabot commented Aug 18, 2019

CLA assistant check
All committers have signed the CLA.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Aug 18, 2019
@ark120202 ark120202 force-pushed the no-useless-computed-key-class-methods branch from 09e0065 to 9cfc1a3 Compare August 18, 2019 01:32
@g-plane g-plane added enhancement This change enhances an existing feature of ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 20, 2019
@mdjermanovic
Copy link
Member

Marking as accepted, as the issue is accepted.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Sep 1, 2019
@mdjermanovic
Copy link
Member

A couple of edge cases.

  1. static ["constructor"]

class A { static ["constructor"](){} } seems to be same as class A { static "constructor"(){} }.

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?

  1. static ["prototype"]

class A { static ["prototype"](){} } seems to be a runtime error.
class A { static "prototype"(){} } seems to be a parsing error.

I think that this should be handled (not reported), because it changes behavior.

@ark120202
Copy link
Contributor Author

Thank you for comments!

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?

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.

@mdjermanovic
Copy link
Member

static name and length also seem to have same behavior in non-computed versions.

So, at the moment, only these two are certainly not 'useless' computed keys:

  • non-static ["constructor"]
  • static ["prototype"]

Copy link
Member

@mdjermanovic mdjermanovic left a 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!

tests/lib/rules/no-useless-computed-key.js Outdated Show resolved Hide resolved
@ark120202 ark120202 force-pushed the no-useless-computed-key-class-methods branch from 3f50a5f to 90158b0 Compare October 8, 2019 19:21
Copy link
Member

@mdjermanovic mdjermanovic left a 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.

tests/lib/rules/no-useless-computed-key.js Show resolved Hide resolved
docs/rules/no-useless-computed-key.md Outdated Show resolved Hide resolved
docs/rules/no-useless-computed-key.md Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

I forgot this - could you please update the PR title with the new option's name

@ark120202 ark120202 force-pushed the no-useless-computed-key-class-methods branch from b30843b to bd117a4 Compare October 30, 2019 17:01
@ark120202 ark120202 changed the title Update: Add checkMethods option to no-useless-computed-key Update: Add enforceForClassMembers option to no-useless-computed-key Oct 30, 2019
@ark120202
Copy link
Contributor Author

@mdjermanovic Sorry for the delay and thanks for your comments

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Copy link
Member

@kaicataldo kaicataldo left a 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 aladdin-add self-requested a review November 13, 2019 10:30
@btmills
Copy link
Member

btmills commented Nov 21, 2019

@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.

Copy link
Member

@platinumazure platinumazure left a 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!

@aladdin-add aladdin-add removed their request for review November 22, 2019 01:40
@aladdin-add
Copy link
Member

please feel free to merge it, since I don't have a time to make a review atm. sorry for the inconvenience!

@kaicataldo kaicataldo merged commit 4f8a1ee into eslint:master Nov 22, 2019
@kaicataldo
Copy link
Member

Thanks for contributing!

@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators May 22, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-useless-computed-key should check class members
9 participants