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 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 eslint bot added the triage label Aug 18, 2019
@ark120202 ark120202 force-pushed the ark120202:no-useless-computed-key-class-methods branch from 09e0065 to 9cfc1a3 Aug 18, 2019
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 1, 2019

Marking as accepted, as the issue is accepted.

@mdjermanovic mdjermanovic added accepted and removed evaluating labels Sep 1, 2019
@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 2, 2019

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

ark120202 commented Sep 2, 2019

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

mdjermanovic commented Sep 2, 2019

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 left a comment

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 ark120202:no-useless-computed-key-class-methods branch from 3f50a5f to 90158b0 Oct 8, 2019
Copy link
Member

mdjermanovic left a comment

Thanks for the changes! I left a couple of notes.

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

mdjermanovic commented Oct 16, 2019

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

@ark120202 ark120202 force-pushed the ark120202:no-useless-computed-key-class-methods branch from b30843b to bd117a4 Oct 30, 2019
@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

ark120202 commented Oct 30, 2019

@mdjermanovic Sorry for the delay and thanks for your comments

Copy link
Member

mdjermanovic left a comment

LGTM, thanks!

Copy link
Member

kaicataldo left a comment

LGTM, thanks!

@aladdin-add aladdin-add self-requested a review Nov 13, 2019
@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 left a comment

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 Nov 22, 2019
@aladdin-add
Copy link
Member

aladdin-add commented Nov 22, 2019

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
18 checks passed
18 checks passed
Verify Files
Details
Test (ubuntu-latest, 13.x)
Details
Test (ubuntu-latest, 12.x)
Details
Test (ubuntu-latest, 10.x)
Details
Test (ubuntu-latest, 8.x)
Details
Test (ubuntu-latest, 8.10.0)
Details
Test (windows-latest, 12.x)
Details
Test (macOS-latest, 12.x)
Details
Browser Test
Details
commit-message PR title follows commit message guidelines
Details
continuous-integration Build #20191030.5 succeeded
Details
continuous-integration (Test on Node.js 10 (Linux)) Test on Node.js 10 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Linux)) Test on Node.js 12 (Linux) succeeded
Details
continuous-integration (Test on Node.js 12 (Windows)) Test on Node.js 12 (Windows) succeeded
Details
continuous-integration (Test on Node.js 12 (macOS)) Test on Node.js 12 (macOS) succeeded
Details
continuous-integration (Test on Node.js 8 (Linux)) Test on Node.js 8 (Linux) succeeded
Details
licence/cla Contributor License Agreement is signed.
Details
release-monitor No patch release is pending
Details
@kaicataldo
Copy link
Member

kaicataldo commented Nov 22, 2019

Thanks for contributing!

@eslint eslint bot locked and limited conversation to collaborators May 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

9 participants
You can’t perform that action at this time.