Skip to content
This repository has been archived by the owner on Jan 19, 2019. It is now read-only.

The future of casing rules #185

Closed
bradzacher opened this issue Nov 26, 2018 · 6 comments
Closed

The future of casing rules #185

bradzacher opened this issue Nov 26, 2018 · 6 comments
Labels
discussion issues that exist purely for discussion purposes

Comments

@bradzacher
Copy link
Owner

I just wanted to spin this off of the discussion that @weirdpattern and I were having on #183.

Currently eslint core has the camelcase rule, which enforces variable and (optionally) property names are written in camelCase or PascalCase.

The problem is that camelcase doesn't differentiate between camelCase and PascalCase (eslint/eslint#10473), and rightfully so because there are different use cases which need different casing (i.e. classes and functional components use PascalCase, variables use camelCase).

This relaxed rule leads to invalid naming patterns that lint okay:

const Pascal = "loophole";
class camel {
  PascalField = "I'm not stage 4 yet"; 
}

To help address this, within this plugin, we have:

  • class-name-casing which only allows PascalCase for classes AND interfaces.
  • member-naming which uses regex on specific modifiers (only private atm).
  • generic-type-naming which uses regex.
  • interface-name-prefix which just allows or blocks I at the start of interface names.
  • camelcase (via Fix: camelcase false positives on interface properties (fixes #177) #183) which adds support for class properties and interface properties on top of the base implementation.

This is obviously a bit of a mess because the naming is inconsistent, and the configuration styles are inconsistent.

The question is, what should our direction be going forward?
(note for below: when I say casing, I mean camelCase / PascalCase / snake_case. When I say case, I mean interface / var / class / property / type / etc).

  1. one rule per case with a standard base config (i.e. enforce casing), and its own individual options (i.e. interface naming to (dis)allow starting with I, (dis)allow class property names starting with _).
  2. one mega rule for name casing, with config for each code case, and then individual rules for the special cases (the individual options above).

This would probably be for a future (2.0?) release, as 1.0.0 is already rather loaded with the dependency upgrades.

Tagging collaborators for visibility
@macklinu, @j-f1, @weirdpattern

@bradzacher bradzacher added the discussion issues that exist purely for discussion purposes label Nov 26, 2018
@j-f1
Copy link
Collaborator

j-f1 commented Nov 26, 2018

I think the multiple smaller rule style is a good idea, although I’d suggest:

  • variable/property/method (I think the addition of class properties should e contributed to eslint-plugin-babel and we should recommend people use that instead)
  • type (e.g. type Foo = Bar)
  • interface
  • namespace/module (ignoring quoted modules e.g. module "foo" {}) (this could be skipped if it’s rarely used)
  • class (should we contribute this to ESLint core instead?)

@j-f1
Copy link
Collaborator

j-f1 commented Nov 26, 2018

I’m wondering if we should collaborate with eslint-plugin-flowtype (cc @gajus since you’re the maintainer of that project) to make an ESLint plugin for features that are common to both Flow and TS.

@weirdpattern
Copy link
Collaborator

@j-f1, does this force users to use babel (maybe is just the parser, dunno)? I know babel now supports TypeScript, but some still prefer to use TypeScript for the transpilation...

variable/property/method (I think the addition of class properties should e contributed to eslint-plugin-babel and we should recommend people use that instead)

@j-f1
Copy link
Collaborator

j-f1 commented Nov 26, 2018

does this force users to use babel

I don’t think so. The Babel parser is in babel-eslint.

@bradzacher
Copy link
Owner Author

my only issue with pushing class properties to a different plugin is that then our users have to also install the plugin to get the coverage.
Telling a typescript user (who doesn't need babel) to use eslint-plugin-babel for a rule would be confusing.
(looking at that plugin, it doesn't need babel, it's just a reimplementation of the rules based off the AST provided by babel-eslint, so there's probably no reason a typescript user who doesn't use babel couldn't use it, assuming it's compatible with typescript-eslint-parser's AST).

Creating a shared library to use between us and eslint-plugin-flowtype would be nice, though that does rely on typescript-eslint-parser keeping the AST we interact with / care about the same. Also depends on if the use cases are the same.

It seems like we're diverging from eslint-core here, so it's hard to say what we should push upstream.
Considering they already have camelcase, would they want extra rule(s) that do similar things to camelcase?
Looking through their rules, the only thing I can see they have for naming other than camelcase is new-cap.

Just to clarify - the idea here would be to essentially tell our users to disable camelcase, and instead use our suite of naming rules.

@weirdpattern
Copy link
Collaborator

disabling camelcase shouldn't be a problem once we have a typescript:recommended preset (which is a whole different topic being discussed in a separate bug...), but yeah, I think we shouldn't force our users to install another plugin just to deal with a single case... I like the idea of the shared library though (and if I remember correctly it was discussed at some point... not sure what happened)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
discussion issues that exist purely for discussion purposes
Projects
None yet
Development

No branches or pull requests

3 participants