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 enforceInMethodNames option to no-underscore-dangle (fixes #7065) #7234

Merged
merged 1 commit into from Jul 16, 2017

Conversation

Projects
None yet
@gabro
Copy link
Contributor

commented Sep 24, 2016

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:
#7065

Please check each item to ensure your pull request is ready:

  • I've read the pull request guide
  • I've included tests for my change
  • I've updated documentation for my change (if appropriate)

What changes did you make? (Give an overview)
When enforceInMethodNames is true, the rule checks for dangling
underscores in method names too. This includes methods of classes
and method properties of objects.

The rule is false by default, in order to preserve backward compatibility.

Is there anything you'd like reviewers to focus on?
Nothing in particular.

@mention-bot

This comment has been minimized.

Copy link

commented Sep 24, 2016

@gabro, thanks for your PR! By analyzing the annotation information on this pull request, we identified @peteward44, @gyandeeps and @vitorbal to be potential reviewers

@eslintbot

This comment has been minimized.

Copy link

commented Sep 24, 2016

Thanks for the pull request, @gabro! I took a look to make sure it's ready for merging and found some changes are needed:

  • The commit summary must be 72 characters or shorter. Please check out our guide for how to properly format your commit summary and update it on this pull request.

Can you please update the pull request to address these?

(More information can be found in our pull request guide.)

@jquerybot

This comment has been minimized.

Copy link

commented Sep 24, 2016

Thank you for your pull request. It looks like this may be your first contribution to a jQuery Foundation project, if so we need you to sign our Contributor License Agreement (CLA).

📝 Please visit http://contribute.jquery.org/CLA/ to sign.

After you signed, the PR is checked again automatically after a minute. If there's still an issue, please reply here to let us know.


If you've already signed our CLA, it's possible your git author information doesn't match your CLA signature (both your name and email have to match), for more information, check the status of your CLA check.

@gabro gabro force-pushed the gabro:master branch from b63662e to a968c86 Sep 24, 2016

@jquerybot jquerybot added CLA: Valid and removed CLA: Error labels Sep 24, 2016

@@ -42,6 +42,7 @@ This rule has an object option:
* `"allow"` allows specified identifiers to have dangling underscores
* `"allowAfterThis": false` (default) disallows dangling underscores in members of the `this` object
* `"allowAfterSuper": false` (default) disallows dangling underscores in members of the `super` object
* `"enforceInMethodNames": false (default) allows dangling underscores in method names`

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 25, 2016

Contributor

seems like if the name is "enforce" then it _dis_allows dangling underscores?

This comment has been minimized.

Copy link
@gabro

gabro Sep 25, 2016

Author Contributor

that would make sense, but I was confused by the other options.

allowAfterSuper => disallows?

I assumed the description referred to the effect of the default option.

This comment has been minimized.

Copy link
@ljharb

ljharb Sep 25, 2016

Contributor

ah, i hadn't read it that way. sounds good :-)

@vitorbal

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

Hi @gabro, thanks a lot for your contribution.

Could you do us a favor and fill in the template for proposing a rule change? We need all of this information in order to determine whether or not the change is a good candidate for inclusion.

Thanks!

@vitorbal vitorbal added the needs info label Sep 26, 2016

@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Sep 26, 2016

Hi Vitor, I didn't complete the template because I understood it wasn't needed when there's an issue associated to it.
Anyway no problem, I'll fill it in later today!

gabriele
~ buildo

On 26 Sep 2016, 11:18 +0200, Vitor Balocco notifications@github.com, wrote:

Hi @gabro (https://github.com/gabro), thanks a lot for your contribution.

Could you do us a favor and fill in the template for proposing a rule change (https://github.com/eslint/eslint/blob/master/templates/rule-change-proposal.md)? We need all of this information in order to determine whether or not the change is a good candidate for inclusion.

Thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (#7234 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAqO5O-6St-xJiQc1RnYvlBr-U9qIWArks5qt434gaJpZM4KFxmg).

@vitorbal

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

@gabro Oh sorry, that's my bad, I didn't notice there was already an issue associated with it. In that case, please disregard what I said :)

@vitorbal

This comment has been minimized.

Copy link
Member

commented Sep 26, 2016

Marking this as "do not merge" for now until we accept the corresponding issue (#7065)

@nzakas

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

Just a heads up: we have moved to a new CLA checker on pull requests. Even if you've previously signed our CLA, we will need to you sign the new one. To do so, look at the status checks for licence/cla and click the "Details" link. Sorry for the inconvenience.

@nzakas nzakas removed the CLA: Valid label Oct 31, 2016

@alberto

This comment has been minimized.

Copy link
Member

commented Feb 7, 2017

ping @gabro

@gabro gabro force-pushed the gabro:master branch from a968c86 to 5180aac Feb 7, 2017

@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2017

@alberto not sure what the ping was about, but I took the opportunity to rebase the branch on top of master :)

@gabro gabro force-pushed the gabro:master branch 2 times, most recently from 565016b to 401ff81 Feb 7, 2017

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Jun 3, 2017

Thanks for your interest in improving ESLint. Unfortunately, it looks like this issue didn't get enough support from the team and so I'm closing it. While we wish we'd be able to accommodate everyone's requests, we do need to prioritize. We've found that issues failing to reach consensus after a long time tend to never do it, and as such, we close those issues. This doesn't mean the idea isn't interesting, just that it's not something the team can commit to.

@ljharb

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2017

@not-an-aardvark this is a PR, not an issue; and the corresponding issue (#7065) is still open. Shouldn't this PR be left open until that issue is closed?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Jun 3, 2017

Good point, I'll reopen this for now.

@eslint eslint deleted a comment from eslintbot Jun 21, 2017

@eslint eslint deleted a comment from eslintbot Jun 21, 2017

@eslint eslint deleted a comment from eslintbot Jun 21, 2017

function checkForTrailingUnderscoreInMethodDefinition(node) {
const identifier = node.key.name;

if (typeof identifier !== "undefined" && hasTrailingUnderscore(identifier) && enforceInMethodNames) {

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jun 21, 2017

Member

I feel like the enforceInMethodNames guard should come first in this conditional so that hasTrailingUnderscore() isn't called every time.

This comment has been minimized.

Copy link
@gabro

gabro Jun 22, 2017

Author Contributor

it makes sense, I'll change it.

const identifier = node.key.name;
const isMethod = node.method;

if (typeof identifier !== "undefined" && hasTrailingUnderscore(identifier) && enforceInMethodNames && isMethod) {

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jun 21, 2017

Member

Same comment as above.

const identifier = node.key.name;
const isMethod = node.method;

if (typeof identifier !== "undefined" && hasTrailingUnderscore(identifier) && enforceInMethodNames && isMethod) {

This comment has been minimized.

Copy link
@kaicataldo

kaicataldo Jun 21, 2017

Member

Why is the check for isMethod necessary here? It doesn't look like this ever evaluates to falsey in the tests. Additionally, if we can remove this, I believe checkForTrailingUnderscoreInMethodDefinition and checkForTrailingUnderscoreInMethodProperty become the same function and we can just use one function for both MethodDefinition and Property.

This comment has been minimized.

Copy link
@gabro

gabro Jun 22, 2017

Author Contributor

I don't recall the reason (it's been a while since I've implemented this 😅) but I think you're right.

I'll squash the two functions together.

This comment has been minimized.

Copy link
@gabro

gabro Jun 22, 2017

Author Contributor

ah, wait, I think I remembered why this is different. We don't want to report on any property, but only on method properties. So for instance:

const obj = {
  _foo: 'bar', // this is fine
  _bar() { }   // this will trigger the rule
}

The only discriminant between the two is the method property on the Property node.

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jun 21, 2017

Thanks for contributing to ESLint! Just a few comments.

@gabro gabro force-pushed the gabro:master branch from 401ff81 to 171eedf Jun 22, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jun 22, 2017

LGTM

@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 22, 2017

@kaicataldo I've squashed the two functions together, but I've kept the isMethod check, since we're not interested in properties that are not method (at least that's what I got out of the discussion in the issue)

@kaicataldo

This comment has been minimized.

Copy link
Member

commented Jun 22, 2017

@gabro Ah, makes sense, thanks! And appreciate you responding so quickly after the delay on our side. Looks like linting is currently failing - mind taking a look? I'd love to land this!

@gabro gabro force-pushed the gabro:master branch from 171eedf to d8a7577 Jun 23, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jun 23, 2017

LGTM

@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2017

@kaicataldo woops, I missed a dot at the end of the reporter message. I also took the opportunity to rebase so to replicate it locally. It should be ok now.

@gabro gabro force-pushed the gabro:master branch from d8a7577 to 8b2dea6 Jun 23, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jun 23, 2017

LGTM

@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Jun 23, 2017

@kaicataldo the CI seems happy now :)

@gyandeeps

This comment has been minimized.

Copy link
Member

commented Jul 10, 2017

@eslint/eslint-team I think this is good to go. Its been here for almost a year now. We should take action on this.

@kaicataldo
Copy link
Member

left a comment

One last thing, but otherwise this LGTM. Would you mind adding a test for the case you outlined in this comment?

Thanks for contributing!

@gabro gabro force-pushed the gabro:master branch from 8b2dea6 to 521500b Jul 11, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jul 11, 2017

LGTM

@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2017

@kaicataldo done :)

Update: add enforceInMethodNames to no-underscore-dangle (fixes #7065)
When enforceInMethodNames is true, the rule checks for dangling
underscores in method names too. This includes methods of classes
and method properties of objects.

The rule is false by default.

@gabro gabro force-pushed the gabro:master branch from 521500b to 5a6bf73 Jul 11, 2017

@eslintbot

This comment has been minimized.

Copy link

commented Jul 11, 2017

LGTM

@kaicataldo
Copy link
Member

left a comment

Thank you, this LGTM!

@not-an-aardvark not-an-aardvark merged commit 3c231fa into eslint:master Jul 16, 2017

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
@gabro

This comment has been minimized.

Copy link
Contributor Author

commented Jul 17, 2017

🎉

@eslint eslint bot locked and limited conversation to collaborators Feb 6, 2018

@eslint eslint bot added the archived due to age label Feb 6, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.