New rule request: naming conventions of methods #7065

gabro · 2016-09-06T10:08:44Z

I looked for a rule for enforcing naming conventions on methods of a class but I couldn't find any.

As an example, the AirBnb style guide for React forbids prefixing methods with an underscore, but there is no way in ESLint to enforce it.

The rule I'm proposing would be an equivalent of no-underscore-dangle but for methods.

Valid examples

class Foo {
  bar() { }
}

Invalid examples

class Foo {
  _bar() { }
}

The rule has two purposes: enforcing a common style and preventing errors. The rationale behind the error prevention is that prefixing with an underscore may give a false sense of privacy, which in turn can cause mistakes, e.g. breaking changes to a method which was thought to be private.

I have already a working implementation of a more generic version of this rule https://github.com/buildo/eslint-plugin-method-names, which allows specifying a regex to which method names should confirm.

I think at least the underscore-specific version should be in the core rules, to match the no-underscore-dangle rule.

Open questions (if the proposed rule is valid):

should the rule scope be limited at the underscore case or should it be generic and allow a regex to be specified?
- should the rule scope include class properties defining a function? (my plugin currently does).
  E.g.
```
class Foo {
  bar() { } // method, invalid
  _baz = () => {} // class property, method
}
```
would be both invalid. This is a fairly common use case when using React components, in order to bind handlers.

Updated proposal, after the discussion below

no-underscore-dangle will have one additional option:

enforceInMethodNames (default: false)

`enforceInMethodNames`

Examples of correct code for this rule with the { "enforceInMethodNames": true } option:

class Foo {
  foo() {}
}

const o = {
  foo() {}
}

Examples of incorrect code for this rule with the { "enforceInMethodNames": true } option:

class Foo {
  _foo() {}
}

class Foo {
  foo_() {}
}

const o = {
  _foo() {}
}

const o = {
  foo_() {}
}

The rationale between the additional option and its default value is to make this change non-breaking.

The text was updated successfully, but these errors were encountered:

vitorbal · 2016-09-06T16:47:22Z

Could this be simply an extension of the no-underscore-dangle rule instead of a new rule? We could have an option e.g. disallowForMethodNames (or allowForMethodNames with default true).

gabro · 2016-09-06T16:51:52Z

@vitorbal yes I think so, provided that we choose to limit this to underscores.

My current implementation is wider and allows defining a regex to test against. If that's deemed to be interesting we could have a naming-convention rule to rule them all (testing a regex against identifiers, maybe providing some default cases like dangling underscores).

ljharb · 2016-09-06T16:53:20Z

I definitely think no-underscore-dangle should be warning on concise object method names as well as class method names - it seems like an oversight that it doesn't already do that.

gabro · 2016-09-06T16:59:18Z

Concerning the possibility of extending this to arbitrary regexes, I think it wouldn't unreasonable for a team to want to enforce a convention like: no snake case for methods and variables. This would allow forbidding:

class Foo() {
  foo_bar() {}
}

as well as

const foo_bar = 42

which seems impossible with ESLint as of today.

After all, many linters of other languages support matching a regex for enforcing naming conventions.

The reason I'm bringing this up here, is that a "regex-checking" rule is introduced, it would overlap with no-underscore-dangle so this needs to be taken into account.

platinumazure · 2016-09-06T16:59:34Z

@gabro Have you looked at id-match? That supports a regular expression pattern but it might not enforce for method names.

gabro · 2016-09-06T16:59:51Z

@platinumazure totally missed it, let me check

gabro · 2016-09-06T17:11:01Z

@platinumazure apparently id-match checks any Identifier, so it works for class properties and methods! Thanks, I didn't know about it.

That being said, I think it's fairly difficult to use as there's no way of limiting its scope to methods.

Also, it already overlaps with no-underscore-dangle.

So my proposal would be to extend no-underscore-dangle to include methods and class properties (configurable) and leave id-match to handle all the other cases.

Does it sound reasonable?

platinumazure · 2016-09-06T17:16:37Z

@gabro To be honest, I would prefer extending id-match to support different patterns for different types of identifiers. But let's see what others think.

Long term, it would be great to deprecate and remove no-underscore-dangle since it's basically covered by id-match, but I don't think that's going to get a lot of traction with the team.

ljharb · 2016-09-06T17:20:08Z

I don't want to have to configure id-match to prevent underscores :-/

platinumazure · 2016-09-06T17:55:39Z

Hmm... I think I had mixed up no-underscore-dangle and camelcase in my mind. To me, camelcase is definitely redundant. I could see an argument that no-underscore-dangle is actually useful and different since it primarily checks the beginning and end of identifier names. So, okay, I'm no longer gunning to deprecate no-underscore-dangle.

I'm not really sure what the best approach is here, though.

@gabro Could you flesh out your proposal a little bit? What would really help us out is:

Some examples of how the options schema would now look under your proposal; and
A few examples of correct/incorrect code for some of the option permutations.

Thanks in advance!

ljharb · 2016-09-06T17:57:30Z

I also don't want to have to configure id-match to prevent camelcase. Forcing people to use regex for common cases is bad news bears imo.

platinumazure · 2016-09-06T18:04:55Z

Okay, we've gone off track. That's my fault, I apologize, but let's please get back on track.

If we can get a full proposal for extending no-underscore-dangle to optionally support method names and class properties, then the team can evaluate.

Sound good? 😄

gabro · 2016-09-07T09:29:03Z

Sounds good to me! I'll get back to this later today

gabro · 2016-09-07T10:32:22Z

Straw-man proposal (outdated, see below)

no-underscore-dangle will have two additional options:

allowInMethodNames (default: true)
allowInClassProperties (default: true)

`allowInMethodNames`

Examples of correct code for this rule with the { "allowInMethodNames": false } option:

class Foo {
  foo() {}
}

const o = {
  foo() {}
}

Examples of incorrect code for this rule with the { "allowInMethodNames": false } option:

class Foo {
  _foo() {}
}

class Foo {
  foo_() {}
}

const o = {
  _foo() {}
}

const o = {
  foo_() {}
}

`allowInClassProperties`

Examples of correct code for this rule with the { "allowInClassProperties": false } option:

class Foo {
  foo = () => {}
}

class Foo {
  bar = 42
}

Examples of incorrect code for this rule with the { "allowInClassProperties": false } option:

class Foo {
  _foo = () => {}
}

class Foo {
  bar_ = 42
}

The rationale between the additional options and their defaults is to make this change non-breaking.

Let me know what you guys think!

vitorbal · 2016-09-07T11:06:02Z

Thanks, @gabro!

I think allowInMethods could be allowInMethodNames to disambiguate from method invocations. What do you think?

Also, class properties are still stage 2 so I don't think we should/can support them yet.

One last thing, could you add some examples for concise object method names as well?

gabro · 2016-09-07T11:14:40Z

I think allowInMethods could be allowInMethodNames to disambiguate from method invocations. What do you think?

Yes, I agree in disambiguating. allowInMethodNames is fine. Or even allowInMethodDeclarations.

About the class properties matter, is this a general policy of ESLint? My perception is that class properties are quite standard in the React world for binding methods to the class, so this rule would be sort of incomplete without them. After all, babel-eslint supports them, and it seems harmless to include the option (defaulting to true).

I updated my proposal above to include examples of concise object method names and renaming allowInMethods to allowInMethodName.

vitorbal · 2016-09-07T11:20:52Z

Small thing: the class definitions in your proposal have a typo:

- class Foo() {
+ class Foo {

gabro · 2016-09-07T11:22:13Z

woops, too many languages at the same time. That comes from scala :) Thanks, fixed

vitorbal · 2016-09-07T12:35:41Z

About the class properties matter, is this a general policy of ESLint? My perception is that class properties are quite standard in the React world for binding methods to the class, so this rule would be sort of incomplete without them. After all, babel-eslint supports them, and it seems harmless to include the option (defaulting to true).

Unfortunately, code with class properties won't even parse correctly with the default ESLint parser. Example: https://astexplorer.net/#/dBfzkR927u. This is only supported by babel-eslint for now, and we have a policy of only supporting features that have reached stage 4.

I understand this can be frustrating, but a stage 2 proposal is still prone to change, and the committee has even mentioned that the syntax for class properties may change before it's finalized. This means potentially more work for us to keep the parser up-to-date with specs that are not even final yet.

gabro · 2016-09-07T12:37:35Z

I see you point and understand the concern.

I guess I can always roll my own plugin for supporting this while the feature reaches a more mature stage.

Should I go ahead and remove it from the proposal?

vitorbal · 2016-09-07T12:43:22Z

@gabro Yes, I think we can remove it. After that, from my part this looks good! Thanks for the quick replies. Now let's wait to see what the rest of the team thinks :)

gabro · 2016-09-07T12:44:30Z

Straw-man proposal

no-underscore-dangle will have one additional option:

enforceInMethodNames (default: false)

`enforceInMethodNames`

Examples of correct code for this rule with the { "enforceInMethodNames": true } option:

class Foo {
  foo() {}
}

const o = {
  foo() {}
}

Examples of incorrect code for this rule with the { "enforceInMethodNames": true } option:

class Foo {
  _foo() {}
}

class Foo {
  foo_() {}
}

const o = {
  _foo() {}
}

const o = {
  foo_() {}
}

The rationale between the additional option and its default value is to make this change non-breaking.

ljharb · 2016-09-07T16:02:13Z

It's unfortunate to have options where absence is not the same as false :-/ could we invert the option names so that the defaults can be false?

gabro · 2016-09-24T23:46:08Z

Ok, since it seems we reached an agreement here, I've submitted a PR.

vitorbal · 2016-09-26T09:46:25Z

We still need a champion and one more thumbs up from the team to mark this as accepted (assuming me and @platinumazure are the current +1's)

gabro · 2016-10-14T21:29:19Z

@vitorbal hey there, any news? Still missing a thumb up? :)

platinumazure · 2016-10-14T21:52:09Z

@gabro Unfortunately I can't really tell which post is supposed to be the "canonical proposal" anymore. Any chance you could edit the initial post with whatever we're currently agreed on? Then @vitorbal and I can 👍 it and hopefully get a third team member and a champion.

Sorry for losing track of this. It's not much of an excuse, but with 200-ish issues and 40-ish PRs open and only about 5-10 active team members, this is bound to happen more often than we would like. Thanks for following up and we greatly appreciate your patience.

gabro · 2016-10-14T23:05:02Z

@platinumazure thanks for your reply. I updated the original post with my final proposal!

No need to apologize, I understand this is a big project and it's easy to lose track of older issues. Mine was a reminder, not a complaint :) Thanks for your work!

vitorbal · 2016-10-15T08:58:00Z

@gabro thanks for the poke! I'm still 👍 on the updated proposal to add a new option to no-underscore-dangle. Hopefully we can get some more support as @platinumazure mentioned above.

kaicataldo · 2017-01-02T19:45:20Z

It's hard to tell where we stand on this - @not-an-aardvark @vitorbal @platinumazure do you three support this? If so, then we just need a champion. If not, it might unfortunately be time to close this as per our policy.

platinumazure · 2017-01-02T19:54:19Z

I'm 👍-ing this under the assumption that the main goal is to augment no-underscore-dangle to support syntax that didn't exist when the rule was created. I'm not super comfortable with anything more than that (because ESLint shouldn't be opinionated).

gabro · 2017-01-02T20:23:48Z

@platinumazure I can assure that's my intent

vitorbal · 2017-01-02T20:39:06Z

I'm still 👍 to the proposal of enhancing no-underscore-dangle. If we could get one more 👍 , then I will champion.

vitorbal · 2017-01-07T17:18:57Z

@eslint/eslint-team we need one more +1 to mark this as accepted. Here's the final proposal (under "Updated proposal"). Any takers?

gabro · 2017-01-07T17:21:09Z

@vitorbal actually the final proposal is in the issue description, as per @platinumazure suggestion :)

vitorbal · 2017-01-07T17:30:31Z

@gabro oops, thanks, fixed the link!

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

maxnordlund · 2017-04-04T11:57:37Z

I came across this today and was sadden when I saw this turned into no-underscore-dangle. Would you mind changing the title @gabro?

I really would like the original proposal for regex based method/function nameíng conventions. And I'm hesitant to open a new issue basically named the same.

In my case I'm still rocking ES5, and there I'm using something like:

function MyClass() {}

MyClass.prototype.method = function MyClass$method() {}
// All fine and dandy

MyClass.prototype._privateMethod = function MyClass$_privateMethod() {}
                                                 // ^ This is where the camelcase chokes

I can open a new issue when this is renamed, but like to avoid confusing where possible.

not-an-aardvark · 2017-06-03T07:33:15Z

@eslint/eslint-team We need a champion for this proposal, otherwise it's time to close it.

vitorbal · 2017-06-06T21:56:39Z

I will champion this, but we still need one more thumbs-up. Any takers?
The proposal is essentially for updating the no-underscore-dangle rule to also check class methods (under an option defaulted to false).
There's even a PR ready to go: #7234

…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 added the triage An ESLint team member will look at this issue soon label Sep 6, 2016

This was referenced Sep 6, 2016

rule proposal: method names buildo/eslint-config#73

Open

Why are underscore prefixes for internal methods of a React component considered bad? airbnb/javascript#1024

Closed

vitorbal self-assigned this Jun 6, 2017

alberto 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 Jun 7, 2017

not-an-aardvark closed this as completed in 3c231fa 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

New rule request: naming conventions of methods #7065

New rule request: naming conventions of methods #7065

Comments

gabro commented Sep 6, 2016 • edited

Updated proposal, after the discussion below

enforceInMethodNames

vitorbal commented Sep 6, 2016

gabro commented Sep 6, 2016

ljharb commented Sep 6, 2016

gabro commented Sep 6, 2016

platinumazure commented Sep 6, 2016 • edited

gabro commented Sep 6, 2016

gabro commented Sep 6, 2016

platinumazure commented Sep 6, 2016

ljharb commented Sep 6, 2016

platinumazure commented Sep 6, 2016

ljharb commented Sep 6, 2016

platinumazure commented Sep 6, 2016

gabro commented Sep 7, 2016

gabro commented Sep 7, 2016 • edited

Straw-man proposal (outdated, see below)

allowInMethodNames

allowInClassProperties

vitorbal commented Sep 7, 2016

gabro commented Sep 7, 2016 • edited

vitorbal commented Sep 7, 2016

gabro commented Sep 7, 2016

vitorbal commented Sep 7, 2016

gabro commented Sep 7, 2016

vitorbal commented Sep 7, 2016

gabro commented Sep 7, 2016 • edited

Straw-man proposal

enforceInMethodNames

ljharb commented Sep 7, 2016

gabro commented Sep 24, 2016

vitorbal commented Sep 26, 2016

gabro commented Oct 14, 2016

platinumazure commented Oct 14, 2016 • edited

gabro commented Oct 14, 2016

vitorbal commented Oct 15, 2016

kaicataldo commented Jan 2, 2017

platinumazure commented Jan 2, 2017

gabro commented Jan 2, 2017

vitorbal commented Jan 2, 2017

vitorbal commented Jan 7, 2017 • edited

gabro commented Jan 7, 2017

vitorbal commented Jan 7, 2017

maxnordlund commented Apr 4, 2017 • edited

not-an-aardvark commented Jun 3, 2017

vitorbal commented Jun 6, 2017

gabro commented Sep 6, 2016 •

edited

`enforceInMethodNames`

platinumazure commented Sep 6, 2016 •

edited

gabro commented Sep 7, 2016 •

edited

`allowInMethodNames`

`allowInClassProperties`

gabro commented Sep 7, 2016 •

edited

gabro commented Sep 7, 2016 •

edited

`enforceInMethodNames`

platinumazure commented Oct 14, 2016 •

edited

vitorbal commented Jan 7, 2017 •

edited

maxnordlund commented Apr 4, 2017 •

edited