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

New rule request: naming conventions of methods #7065

Closed
gabro opened this issue Sep 6, 2016 · 42 comments
Closed

New rule request: naming conventions of methods #7065

gabro opened this issue Sep 6, 2016 · 42 comments
Assignees
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

Comments

@gabro
Copy link
Contributor

gabro commented Sep 6, 2016

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.

@vitorbal
Copy link
Member

vitorbal commented Sep 6, 2016

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

@vitorbal vitorbal added enhancement This change enhances an existing feature of ESLint rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion and removed triage An ESLint team member will look at this issue soon labels Sep 6, 2016
@gabro
Copy link
Contributor Author

gabro commented Sep 6, 2016

@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
Copy link
Contributor

ljharb commented Sep 6, 2016

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
Copy link
Contributor Author

gabro commented Sep 6, 2016

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
Copy link
Member

platinumazure commented Sep 6, 2016

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

@gabro
Copy link
Contributor Author

gabro commented Sep 6, 2016

@platinumazure totally missed it, let me check

@gabro
Copy link
Contributor Author

gabro commented Sep 6, 2016

@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
Copy link
Member

@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
Copy link
Contributor

ljharb commented Sep 6, 2016

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

@platinumazure
Copy link
Member

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
Copy link
Contributor

ljharb commented Sep 6, 2016

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
Copy link
Member

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
Copy link
Contributor Author

gabro commented Sep 7, 2016

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

@gabro
Copy link
Contributor Author

gabro commented Sep 7, 2016

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
Copy link
Member

vitorbal commented Sep 7, 2016

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
Copy link
Contributor Author

gabro commented Sep 7, 2016

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
Copy link
Member

vitorbal commented Sep 7, 2016

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

- class Foo() {
+ class Foo {

@gabro
Copy link
Contributor Author

gabro commented Sep 7, 2016

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

@vitorbal
Copy link
Member

vitorbal commented Sep 7, 2016

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
Copy link
Contributor Author

gabro commented Sep 7, 2016

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
Copy link
Member

vitorbal commented Sep 7, 2016

@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
Copy link
Contributor Author

gabro commented Sep 7, 2016

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
Copy link
Contributor

ljharb commented Sep 7, 2016

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
Copy link
Contributor Author

gabro commented Sep 24, 2016

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

@vitorbal
Copy link
Member

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
Copy link
Contributor Author

gabro commented Oct 14, 2016

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

@platinumazure
Copy link
Member

platinumazure commented Oct 14, 2016

@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
Copy link
Contributor Author

gabro commented Oct 14, 2016

@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
Copy link
Member

@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
Copy link
Member

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
Copy link
Member

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
Copy link
Contributor Author

gabro commented Jan 2, 2017

@platinumazure I can assure that's my intent

@vitorbal
Copy link
Member

vitorbal commented Jan 2, 2017

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

@vitorbal
Copy link
Member

vitorbal commented Jan 7, 2017

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

@gabro
Copy link
Contributor Author

gabro commented Jan 7, 2017

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

@vitorbal
Copy link
Member

vitorbal commented Jan 7, 2017

@gabro oops, thanks, fixed the link!

gabro added a commit to gabro/eslint that referenced this issue Feb 7, 2017
…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 added a commit to gabro/eslint that referenced this issue Feb 7, 2017
…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 added a commit to gabro/eslint that referenced this issue Feb 7, 2017
…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
Copy link
Contributor

maxnordlund commented Apr 4, 2017

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
Copy link
Member

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

@vitorbal
Copy link
Member

vitorbal commented Jun 6, 2017

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

@vitorbal vitorbal self-assigned this Jun 6, 2017
@alberto 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
gabro added a commit to gabro/eslint that referenced this issue Jun 22, 2017
…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 added a commit to gabro/eslint that referenced this issue Jun 23, 2017
…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 added a commit to gabro/eslint that referenced this issue Jun 23, 2017
…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 added a commit to gabro/eslint that referenced this issue Jul 11, 2017
…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 added a commit to gabro/eslint that referenced this issue Jul 11, 2017
…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.
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 6, 2018
@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 Feb 6, 2018
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

No branches or pull requests

9 participants