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

[explicit-member-accessibility]: autofix #503

Closed
thejuan opened this issue May 9, 2019 · 12 comments
Closed

[explicit-member-accessibility]: autofix #503

thejuan opened this issue May 9, 2019 · 12 comments
Labels
enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin wontfix This will not be worked on

Comments

@thejuan
Copy link

thejuan commented May 9, 2019

Feature Request for autofixing for rule explicit-member-accessibility
Its currently possible with the TSLint plugin: member-access and makes live easier by defaulting everything to public (based on settings)

@thejuan thejuan added package: eslint-plugin-tslint Issues related to @typescript-eslint/eslint-plugin-tslint triage Waiting for maintainers to take a look labels May 9, 2019
@bradzacher bradzacher added enhancement New feature or request package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin and removed package: eslint-plugin-tslint Issues related to @typescript-eslint/eslint-plugin-tslint triage Waiting for maintainers to take a look labels May 9, 2019
@bradzacher
Copy link
Member

We've talked about this in the past.
a fixer is super simple to make sure, but its really bad for this rule.

The entire point of the rule is so that you think about the accessibility of your members.
If there is a fixer which automatically sets everything to public, then that is the exact opposite of thinking about your member accessibility.

Of course, there are people that use this rule purely as a consistency check.
If there's enough demand from the community, happy to look at adding one.

@SalvatorePreviti
Copy link

SalvatorePreviti commented Jun 2, 2019

I use eslint and prettier to cleanup and format the --declaration *.d.ts output generated after compilation. It would be really helpful to allow the autofix for this rule at least on .d.ts files or with a flag.

@Devilsparta
Copy link

Devilsparta commented Nov 20, 2019

need a fixer too, or how can I just write one for myself

@bradzacher
Copy link
Member

happy to accept a PR.
there's plenty of example code in this repo.

@ghost
Copy link

ghost commented Dec 17, 2019

unwantedPublicAccessibility could be fixed without any problem

@pablobirukov
Copy link
Contributor

pablobirukov commented Jan 30, 2020

I don't need an autofixer that just adds public, but I'd like to have one when I introduce the rule in a new codebase. For example, if I want to prefix all constructors.

How about a fixer that is enabled under a flag in rule options? That way you can turn it on for short time, prefix required nodes and turn off so that developers won't accidentally put public everywhere on every save of a document.

Maybe something very explicit, like this?

interface Config {
  // ...
  overrides?: {
    accessors?: AccessibilityLevel;
    constructors?: AccessibilityLevel;
    // ...
  };
  UNSAFE_AUTOFIX: true | Array<keyof NonNullable<Config["overrides"]>>;
}

@bradzacher
Copy link
Member

Happy for someone to add suggestion fixers to this rule:
https://eslint.org/docs/developer-guide/working-with-rules#providing-suggestions

Suggestion fixers are not automatic, but IDEs will show them as possible to fix.
You can have multiple suggestion fixers, so that way we could have one suggestion per modifier, and the user must choose which one.

@pablobirukov
Copy link
Contributor

Looks exactly what I needed, thanks. Will provide a PR shortly.

I'm I right these can be applied with --fix-type passed?

@bradzacher
Copy link
Member

I'm I right these can be applied with --fix-type passed?

No; suggestion fixers cannot be applied via the CLI at all. Only manually via an IDE.
They may change that in the future, but right now it's not on the roadmap AFAIK.

It's a bit confusing yeah, because they used the same terminology unfortunately.
When you create a rule, you can add a property to its metadata (rule.meta.type) which may be either "problem" | "layout" | "suggestion".
This property is unrelated to whether or not the rule has a suggestion fixer. It's more of a categorisation for the rule.

The --fix-type flag will limit the fixers that are run on your codebase to rules that have rule.meta.type property set to the specified type.

@crazytoucan
Copy link

crazytoucan commented May 24, 2020

Hey, it's a little disappointing that you don't want to accept a fixer that adds public -- I think there are legitimate uses for such a thing. I am sympathetic to the argument that defaulting everything to being public is against the spirit of selecting member visibility, but as somebody who inherits codebases that we're trying to make consistent, the auto fixer would go a long way to just normalizing the code, especially where they used a lack of visibility modifier to mean public and were otherwise consistent in their usage of private.

@bradzacher
Copy link
Member

If your usecase is normalising a codebase, then you're not looking for a lint rule - you want a codemod, which is pretty trivial to do using a tool like jscodeshift.

Push comes to shove, it would also be relatively low effort to patch a fixer locally into the rule using a tool like patch-package.

Also worth noting that if someone adds suggestion fixers, then it would be easy enough to then write a script using ESLint's API that runs the rule, collects and then applies the fixes.

Will gladly accept a PR to add suggestion fixers, but a full auto fixer will not be added.

@bradzacher bradzacher added the wontfix This will not be worked on label Aug 7, 2020
@bradzacher
Copy link
Member

I'm going to reject this request for the following reasons:

(1) it goes against the spirit of the rule.

As with explicit-function-return-types, this rule exists so that developers think about the contracts they are building.

Automatically fixing all methods to be public does not do that.

(2) fixers live for the lifetime of the rule, not just the the initial onboarding

Adding a fixer to help onboard users seems like a great idea on the surface, but it is actually a really bad idea.

If you add a fixer that's always on, then users will just use the fixer and not think about it, so that's bad as per (1).

(2)(a) make a configurable fixer

If you make a configurable fixer, then you're relying on users to turn the fixer off once they've onboarded their codebase. Having seen many codebases and onboarding processes, I know that this is not likely to happen - users will more than likely forget to turn the fixer off once onboarded, which leads back to (1).

(3) the only safe fix is to fix to public.

ESLint fixers have to be safe. That means that ideally there should be no build or runtime errors introduced into the codebase by running a fixer.

This means it's impossible for a fixer for this rule to do anything but add the public modifier, because private or protected will break the build if the member is used outside the class.

(3)(a) making a "smart" fixer requires type information.

This of course brings up the idea that the rule could use type information to find usages of a member so it can default to the strictest possible accessibility.

There are two problems with this:

  1. detecting usage in typescript is incredibly complex (eg feat(eslint-plugin): add rule no-unused-exports #1905), so that's a lot of code for us to maintain
  2. introducing a type information into a rule is a major breaking change, which severely limits the usability of the rule for many users, because many of our users don't use type information for various reasons.

I understand that there may be some convenience to having a fixer for those that are adding the rule to a codebase, but a fixer isn't just a codemod tool - it lives on, and runs forever with the rule.

It's for these reasons like this that rules like no-unused-vars don't have fixers either - it might be good to cleanup a codebase, but it's not good during development.

If you need to onboard your codebase onto this rule, consider using a codemod tool to update your codebase as you wish.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request 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

6 participants