Update: add enforceInMethodNames option to no-underscore-dangle (fixes #7065) #7234

gabro · 2016-09-24T23:42:19Z

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 · 2016-09-24T23:42:20Z

@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 · 2016-09-24T23:42:21Z

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 · 2016-09-24T23:42:23Z

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.

ljharb · 2016-09-25T00:23:32Z

docs/rules/no-underscore-dangle.md

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


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

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.

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

vitorbal · 2016-09-26T09:18:42Z

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!

gabro · 2016-09-26T09:21:00Z

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 · 2016-09-26T09:21:50Z

@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 · 2016-09-26T09:23:09Z

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

nzakas · 2016-10-31T20:21:55Z

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.

alberto · 2017-02-07T04:18:37Z

ping @gabro

gabro · 2017-02-07T18:24:37Z

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

not-an-aardvark · 2017-06-03T07:03:20Z

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 · 2017-06-03T07:31:05Z

@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 · 2017-06-03T07:32:54Z

Good point, I'll reopen this for now.

kaicataldo · 2017-06-21T22:27:24Z

lib/rules/no-underscore-dangle.js

+        function checkForTrailingUnderscoreInMethodDefinition(node) {
+            const identifier = node.key.name;
+
+            if (typeof identifier !== "undefined" && hasTrailingUnderscore(identifier) && enforceInMethodNames) {


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

it makes sense, I'll change it.

kaicataldo · 2017-06-21T22:33:07Z

lib/rules/no-underscore-dangle.js

+            const identifier = node.key.name;
+            const isMethod = node.method;
+
+            if (typeof identifier !== "undefined" && hasTrailingUnderscore(identifier) && enforceInMethodNames && isMethod) {


Same comment as above.

kaicataldo · 2017-06-21T22:35:23Z

lib/rules/no-underscore-dangle.js

+            const identifier = node.key.name;
+            const isMethod = node.method;
+
+            if (typeof identifier !== "undefined" && hasTrailingUnderscore(identifier) && enforceInMethodNames && isMethod) {


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.

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.

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 · 2017-06-21T22:36:30Z

Thanks for contributing to ESLint! Just a few comments.

eslintbot · 2017-06-22T08:18:46Z

LGTM

gabro · 2017-06-22T08:21:19Z

@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 · 2017-06-22T14:47:05Z

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

eslintbot · 2017-06-23T12:14:13Z

LGTM

gabro · 2017-06-23T12:14:39Z

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

eslintbot · 2017-06-23T13:16:05Z

LGTM

gabro · 2017-06-23T13:30:54Z

@kaicataldo the CI seems happy now :)

gyandeeps · 2017-07-10T15:11:48Z

@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

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

Thanks for contributing!

eslintbot · 2017-07-11T13:25:49Z

LGTM

gabro · 2017-07-11T13:25:55Z

@kaicataldo done :)

…t#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.

eslintbot · 2017-07-11T13:27:46Z

LGTM

kaicataldo

Thank you, this LGTM!

gabro · 2017-07-17T16:59:27Z

🎉

jquerybot added the CLA: Error label Sep 24, 2016

gabro force-pushed the master branch from b63662e to a968c86 Compare September 24, 2016 23:44

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

ljharb reviewed Sep 25, 2016

View reviewed changes

vitorbal added the needs info Not enough information has been provided to triage this issue label Sep 26, 2016

vitorbal added do not merge This pull request should not be merged yet and removed needs info Not enough information has been provided to triage this issue labels Sep 26, 2016

nzakas removed the CLA: Valid label Oct 31, 2016

gabro force-pushed the master branch from a968c86 to 5180aac Compare February 7, 2017 18:23

gabro force-pushed the master branch 2 times, most recently from 565016b to 401ff81 Compare February 7, 2017 18:31

not-an-aardvark closed this Jun 3, 2017

eslint deleted a comment from eslintbot Jun 21, 2017

kaicataldo suggested changes Jun 21, 2017

View reviewed changes

gabro force-pushed the master branch from 401ff81 to 171eedf Compare June 22, 2017 08:18

gabro force-pushed the master branch from 171eedf to d8a7577 Compare June 23, 2017 12:14

gabro force-pushed the master branch from d8a7577 to 8b2dea6 Compare June 23, 2017 13:16

kaicataldo suggested changes Jul 11, 2017

View reviewed changes

gabro force-pushed the master branch from 8b2dea6 to 521500b Compare July 11, 2017 13:25

Update: add enforceInMethodNames to no-underscore-dangle (fixes eslin…

5a6bf73

…t#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 force-pushed the master branch from 521500b to 5a6bf73 Compare July 11, 2017 13:27

kaicataldo approved these changes Jul 11, 2017

View reviewed changes

not-an-aardvark approved these changes Jul 16, 2017

View reviewed changes

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

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

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 6, 2018

Update: add enforceInMethodNames option to no-underscore-dangle (fixes #7065) #7234

Update: add enforceInMethodNames option to no-underscore-dangle (fixes #7065) #7234

Conversation

gabro commented Sep 24, 2016 • edited

mention-bot commented Sep 24, 2016

eslintbot commented Sep 24, 2016

jquerybot commented Sep 24, 2016

ljharb Sep 25, 2016

Choose a reason for hiding this comment

gabro Sep 25, 2016 • edited

Choose a reason for hiding this comment

ljharb Sep 25, 2016

Choose a reason for hiding this comment

vitorbal commented Sep 26, 2016

gabro commented Sep 26, 2016

vitorbal commented Sep 26, 2016

vitorbal commented Sep 26, 2016

nzakas commented Oct 31, 2016

alberto commented Feb 7, 2017

gabro commented Feb 7, 2017

not-an-aardvark commented Jun 3, 2017

ljharb commented Jun 3, 2017

not-an-aardvark commented Jun 3, 2017

kaicataldo Jun 21, 2017

Choose a reason for hiding this comment

gabro Jun 22, 2017

Choose a reason for hiding this comment

kaicataldo Jun 21, 2017 • edited

Choose a reason for hiding this comment

kaicataldo Jun 21, 2017

Choose a reason for hiding this comment

gabro Jun 22, 2017

Choose a reason for hiding this comment

gabro Jun 22, 2017

Choose a reason for hiding this comment

kaicataldo commented Jun 21, 2017

eslintbot commented Jun 22, 2017

gabro commented Jun 22, 2017

kaicataldo commented Jun 22, 2017

eslintbot commented Jun 23, 2017

gabro commented Jun 23, 2017 • edited

eslintbot commented Jun 23, 2017

gabro commented Jun 23, 2017

gyandeeps commented Jul 10, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

eslintbot commented Jul 11, 2017

gabro commented Jul 11, 2017

eslintbot commented Jul 11, 2017

kaicataldo left a comment

Choose a reason for hiding this comment

gabro commented Jul 17, 2017

gabro commented Sep 24, 2016 •

edited

gabro Sep 25, 2016 •

edited

kaicataldo Jun 21, 2017 •

edited

gabro commented Jun 23, 2017 •

edited