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

Rule proposal: prefer hash names for private members #1391

Closed
kimamula opened this issue Dec 30, 2019 · 11 comments
Closed

Rule proposal: prefer hash names for private members #1391

kimamula opened this issue Dec 30, 2019 · 11 comments
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@kimamula
Copy link

TypeScript 3.8 will add support for private named instance fields. What about creating a rule for disallowing the use of the private accessibility modifier in favor of the standard hash names for private members?

Examples of incorrect code for this rule:

class MyClass {
  private foo = 1;
  constructor(private bar: string) {}
}

Examples of correct code for this rule:

class MyClass {
  #foo = 1;
  #bar: string;
  constructor(bar: string) {
    this.#bar = bar;
  }
}

Options

// Any target whose hash names are supported
// by the TypeScript version in use can be specified
type Target =
//  | 'accessors'
//  | 'constructors'
//  | 'staticMethods'
//  | 'instanceMethods'
//  | 'staticFields'
  | 'instanceFields';

type Options = Target[];

// Defaults to all the targets
const defaultOptions: Options = ['instanceFields']; // for TypeScript 3.8
@kimamula kimamula added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Dec 30, 2019
@glen-84
Copy link
Contributor

glen-84 commented Dec 30, 2019

Perhaps it should allow the opposite as well – enforcing soft (compile-time-only) privacy?

@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for team members to take a look labels Dec 30, 2019
@bradzacher
Copy link
Member

Probably best to be added as an option on the explicit-member-accessibility rule.

@kimamula
Copy link
Author

kimamula commented Dec 31, 2019

Then the options for explicit-member-accessibility should be changed to something like the following?

type AccessibilityLevel =
  | 'explicit' // require an accessor (including public)
  | 'no-public' // don't require public
  | 'off'; // don't check

type PrivateMemberDeclaration =
  | 'private' // force private modifier
  | '#' // force hash names
  | 'off'; // don't check, default

type AccessibilitySetting = AccessibilityLevel | [AccessibilityLevel, PrivateMemberDeclaration];

type Options = {
  accessibility?: AccessibilitySetting;
  ignoredMethodNames?: string[];
  overrides?: {
    accessors?: AccessibilitySetting;
    constructors?: AccessibilitySetting;
    methods?: AccessibilitySetting | {
      static?: AccessibilitySetting;
      instance?: AccessibilitySetting;
    };
    properties?: AccessibilitySetting | {
      static?: AccessibilitySetting;
      instance?: AccessibilitySetting;
    };
    parameterProperties?: AccessibilitySetting;
  };
};

const defaultOptions: Options = {
  accessibility: 'explicit',
};

I'm afraid that the name explicit-member-accessibility is a bit confusing for this use case though.
Perhaps it should be renamed to something like member-accessibility and the name explicit-member-accessibility should remain as an alias for it?

@zheeeng
Copy link

zheeeng commented Mar 1, 2022

Expect more discussions and progress on this issue.

@bradzacher
Copy link
Member

With any issue in opened in this project - it either has a visible progress in the form of an attached PR, or it has no progress.

We are a community run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it.

If this issue is important to you - consider being that champion.

If not - please just subscribe to the issue and wait patiently.
Commenting asking for status updates does not bump issue priority in any way and just serves to spam everyone subscribed to the issue.

@npdev453
Copy link

npdev453 commented May 11, 2022

Current workaround:

'@typescript-eslint/naming-convention': [
  'error',
  {
    selector: ['property', 'parameterProperty', 'accessor'],
    modifiers: ['private'],
    prefix: ['#'],
    format: null,
  }
],

@bradzacher
Copy link
Member

As per our FAQ article - this can easily be achieved using no-restricted-syntax:

no-restricted-syntax: [
    'error',
    {
        selector: ':matches(PropertyDefinition, MethodDefinition)[accessibility="private"]',
        message: 'Use #private instead',
    },
],

playground

similarly the inverse can easily be enforced as well:

no-restricted-syntax: [
    'error',
    {
        selector: ':matches(PropertyDefinition, MethodDefinition) > PrivateIdentifier.key',
        message: 'Use `private` instead',
    },
]

playground

I don't believe that a lint rule makes sense here. We want to avoid adding rules which just ban a specific language features unless we can add some real additional value. Given that there's no fixer we can provide (because it's unsafe), and there's no options to configure or tweak - I don't see any additional value we can provide.

For this reason, we shall reject this rule proposal.

@bradzacher bradzacher added wontfix This will not be worked on and removed accepting prs Go ahead, send a pull request that resolves this issue labels May 11, 2022
@thetutlage
Copy link

I don't believe that a lint rule makes sense here. We want to avoid adding rules which just ban a specific language features unless we can add some real additional value

I don't think we are banning language features here. Infact, we are asking to promote the language feature ie private properties

@bradzacher
Copy link
Member

I don't think we are banning language features here. Infact, we are asking to promote the language feature ie private properties

What's the difference to a linter?
In reality - there's no difference. A lint rule will report on any class member that uses private. There's no way to subtly suggest - it will just be a lint error on every single private member.

In practice this is exactly the same as "banning" private altogether.

@thetutlage
Copy link

thetutlage commented May 12, 2022

private is a modifier and # is a private property. Both are different things all together.

All we are asking is to enforce the usage of # in favor of private.

In practice this is exactly the same as "banning" private altogether.

Don't we do that already in other places like?

  • Ban double quotes in favor of single quotes
  • Ban usage of var in favor of const and let

@bradzacher
Copy link
Member

bradzacher commented May 12, 2022

Sorry, I think you've misunderstood. My comment above gives you a solution to do exactly what you want

Why would we build a separate rule when there is already existing tooling that more than meets the requirements?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

7 participants