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

feat(eslint-plugin): Make explicit-member-accessibility fixable #331

Conversation

rhysd
Copy link
Contributor

@rhysd rhysd commented Mar 5, 2019

This PR adds the ability to fix errors from explicit-member-accessibility rule automatically by adding public specifier explicitly in code.

I also added more tests for the rule since I wanted to confirm that public must be put before readonly as per TypeScript grammar.

@rhysd rhysd force-pushed the explicit-member-accessibility-fixable branch from f225d74 to 4f07f32 Compare March 5, 2019 08:27
@rhysd
Copy link
Contributor Author

rhysd commented Mar 5, 2019

I did git push -f to fix code formatting and commit comment

@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #331 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #331      +/-   ##
==========================================
+ Coverage   97.27%   97.27%   +<.01%     
==========================================
  Files          67       67              
  Lines        2348     2350       +2     
  Branches      335      335              
==========================================
+ Hits         2284     2286       +2     
  Misses         43       43              
  Partials       21       21
Impacted Files Coverage Δ
...-plugin/src/rules/explicit-member-accessibility.ts 100% <100%> (ø) ⬆️

@rhysd
Copy link
Contributor Author

rhysd commented Mar 6, 2019

@j-f1 I addressed the review comment at 040ba81

@bradzacher
Copy link
Member

Is a fixer for this a good thing?
Esp with tools like prettier-eslint, you could accidentally silence an error and accidentally expose a function when you meant to mark it private.

IMO There are two points to this rule:

  1. ensure you explicitly provide accessibility.
  2. make you think about providing an accessibility.

The fixer takes care of (1), but it makes it too easy to circumvent (2).

I'm not sure if we should be providing fixers for things like this that can have a greater effect on how your code works.

@rhysd
Copy link
Contributor Author

rhysd commented Mar 7, 2019

@bradzacher

That's good point. I thought the same while writing tests for this and actually I have not good answer for it.
It would be dangerous to run --fix without checking warnings/errors. I'm not sure it is the normal use case.

@bradzacher
Copy link
Member

bradzacher commented Mar 8, 2019

I'd like to hear thoughts from @typescript-eslint/core-team in regards to the addition of a fixer here.

If we do want a fixer here, maybe it'd be worth making it default to private (so that it can be fixed, but so that it doesn't default to the most open modifier)?
Or would it be better to make it configurable, so the user can decide?

Note There's also #322 which is touching this rule, which will make it possible to ban using public. That option makes sense to fix, as it's like-for-like - you either define a modifier that isn't public, or you use public and can fix remove it.


It sucks that eslint doesn't have a mechanism for saying "let the user pick one of these fixers" 😢, as that'd be ideal here.

@mysticatea
Copy link
Member

Though I'm not a core member.

I remember that I added the same autofix into the local installed eslint-plugin-typescript and used it to migrate my codebase in the past. But at that time, I had the same concern (autofix of this rule is useful, but it shouldn't fix silently), so I didn't send a PR.

@rhysd
Copy link
Contributor Author

rhysd commented Mar 8, 2019

@bradzacher @mysticatea @j-f1 I changed my thought. I added this since it was useful to migrate my code base with this fixer easily. But I think I should put safety first. I'm closing this. Thank you for your feedbacks and reviews!

@rhysd rhysd closed this Mar 8, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants