feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

jtbandes · 2022-08-17T01:30:57Z

PR Checklist

Addresses an existing ~~open~~ issue: This PR addresses [explicit-member-accessibility] "explicit" option suggestions #4647 (cc @alumni), but the issue was closed.
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

This PR adds auto-fix support to explicit-member-accessibility's default mode (explicit) by automatically adding public to declarations without an existing accessibility level.

Re #4647 (comment):

If we simply autofixed every case to public then it would defeat the purpose of the rule.

I disagree that it defeats the purpose of the rule: the purpose is to make developers think explicitly about accessibility levels, and auto-adding public makes it obvious that the default accessibility is public.

I tested out this change on a large TS codebase: https://github.com/foxglove/studio/pull/4164

Another option, if you find this auto-fix undesirable, would be to make it a suggestion instead of a fix. What do you think?

…accessibility

nx-cloud · 2022-08-17T01:30:59Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 5471380. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 47 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-08-17T01:31:00Z

Thanks for the PR, @jtbandes!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

netlify · 2022-08-17T01:31:01Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`5471380`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/630635be8118fb000822c849
😎 Deploy Preview	https://deploy-preview-5492--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

codecov · 2022-08-17T01:44:43Z

Codecov Report

Merging #5492 (6395b8f) into main (1b2601a) will increase coverage by 2.34%.
The diff coverage is 100.00%.

❗ Current head 6395b8f differs from pull request most recent head 5471380. Consider uploading reports for the commit 5471380 to get more accurate results

@@            Coverage Diff             @@
##             main    #5492      +/-   ##
==========================================
+ Coverage   91.57%   93.92%   +2.34%     
==========================================
  Files         132      290     +158     
  Lines        1496    10024    +8528     
  Branches      226     3016    +2790     
==========================================
+ Hits         1370     9415    +8045     
- Misses         62      330     +268     
- Partials       64      279     +215

Flag	Coverage Δ
unittest	`93.92% <100.00%> (+2.34%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...-plugin/src/rules/explicit-member-accessibility.ts	`97.43% <100.00%> (ø)`
...src/configs/recommended-requiring-type-checking.ts	`100.00% <0.00%> (ø)`
...nt-plugin/src/rules/consistent-type-definitions.ts	`97.29% <0.00%> (ø)`
packages/eslint-plugin/src/rules/indent.ts	`92.85% <0.00%> (ø)`
packages/eslint-plugin/src/rules/unbound-method.ts	`92.13% <0.00%> (ø)`
...ackages/eslint-plugin/src/util/getWrappingFixer.ts	`100.00% <0.00%> (ø)`
...eslint-plugin/src/rules/consistent-type-exports.ts	`97.70% <0.00%> (ø)`
...plugin/src/rules/explicit-module-boundary-types.ts	`91.04% <0.00%> (ø)`
packages/eslint-plugin/src/rules/array-type.ts	`97.14% <0.00%> (ø)`
...ackages/eslint-plugin/src/rules/no-var-requires.ts	`90.00% <0.00%> (ø)`
... and 149 more

bradzacher · 2022-08-17T10:03:40Z

Thanks for the PR.
As per the issue - we will not be accepting autofix support for this rule.

the purpose is to make developers think explicitly about accessibility levels, and auto-adding public makes it obvious that the default accessibility is public.

The issue is that once you autofix to add public, then the rule no longer reports at all.
This is why I say that it defeats the purpose of the rule! If the rule doesn't report then you won't be prompted to think about the accessibility!

So if someone is paying little attention and just runs --fix then they won't ever see the lint report on the member.
Similarly if the user has autofix-on-save turned on then they will add a new member, save the file and public is auto-added - again they never see the lint report to prompt them to consider which modifier is right!

We don't want this rule to have a fixer that people use to codemod their codebase (like you did in your linked PR) - lint features last forever and need to support the normal, non-codemod usecase.

If you'd like to make this a suggestion fixer - I'm happy to accept that. We could have a suggestion fixer for each of the accessibility modifiers.
That would allow people to quickly choose and add the relevant modifier with a single click.

jtbandes · 2022-08-18T01:39:03Z

Updated to suggest 3 fixes for each error, adding public, private, and protected.

Screen.Recording.2022-08-17.at.6.41.16.PM.mov

jtbandes · 2022-08-18T01:42:48Z

And although it's no longer relevant since I've changed them to suggestions, I just want to give my perspective on:

The issue is that once you autofix to add public, then the rule no longer reports at all.
This is why I say that it defeats the purpose of the rule! If the rule doesn't report then you won't be prompted to think about the accessibility!

There's a step where you do still notice the change: code review :)

JoshuaKGoldberg

Started reviewing and found blockers in the codecov report and unifying the message IDs. Left a few other questions too 🙂 . I'll review in more detail (so many new lines of test code! yay!) once those are answered. Thanks for sending this in! ✨

packages/eslint-plugin/src/rules/explicit-member-accessibility.ts

JoshuaKGoldberg · 2022-08-18T15:35:15Z

packages/eslint-plugin/src/rules/explicit-member-accessibility.ts

+          const lastDecorator = node.decorators[node.decorators.length - 1];
+          const nextToken = sourceCode.getTokenAfter(lastDecorator);
+          if (!nextToken) {
+            return null;


We generally expect most new code -and ideally all new rule code- to be covered by tests. Codecov is rightfully showing here that this isn't. One of two cases is true:

This case can be tested and should be

This case is impossible and the type system is wrong

Meaning: use a ! to show that the nextToken does always exist

I expect it is probably impossible, because I can't think of any situation where there wouldn't be any tokens after the last decorator, but it feels wrong to simply ! the output of getTokenAfter just because I believe that (perhaps erroneously), so I was trying to code defensively. But that also means I can't think of any test cases to add test coverage here. What would you recommend doing?

I say ! it. If there are decorators they must be decorating something for this to be valid syntax.

In an ideal world, sourceCode.getTokenAfter would know that if it's given a node that must have an after, the returned token is defined. If we eventually get rich enough types the ! will be linted as unnecessary.

packages/eslint-plugin/src/rules/explicit-member-accessibility.ts

packages/eslint-plugin/tests/rules/explicit-member-accessibility.test.ts

…-accessibility

Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>

…y' into fix-explicit-member-accessibility

jtbandes · 2022-08-18T20:18:09Z

Not sure what's going on with the website tests – could that be related to my changes? Seems unlikely?

jtbandes · 2022-08-23T23:05:56Z

@JoshuaKGoldberg @bradzacher friendly ping, anything else you'd like me to change here?

JoshuaKGoldberg · 2022-08-24T14:33:46Z

Nope this looks great, thanks!

Re the website tests: 🤷 I have given up on them. They do what they want to do. We have no power here.

JoshuaKGoldberg

✨

jtbandes · 2022-08-24T15:32:20Z

Thanks for all the review!

bradzacher · 2022-08-24T15:34:31Z

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.33.1` -> `5.35.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.33.1/5.35.1) | | [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.33.1` -> `5.35.1`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.33.1/5.35.1) | --- ### Release Notes <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/eslint-plugin)</summary> ### [`v5.35.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5351-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5350v5351-2022-08-24) [Compare Source](typescript-eslint/typescript-eslint@v5.35.0...v5.35.1) ##### Bug Fixes - **eslint-plugin:** correct rule schemas to pass ajv validation ([#5531](typescript-eslint/typescript-eslint#5531)) ([dbf8b56](typescript-eslint/typescript-eslint@dbf8b56)) ### [`v5.35.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5350-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5340v5350-2022-08-24) [Compare Source](typescript-eslint/typescript-eslint@v5.34.0...v5.35.0) ##### Features - **eslint-plugin:** \[explicit-member-accessibility] suggest adding explicit accessibility specifiers ([#5492](typescript-eslint/typescript-eslint#5492)) ([0edb94a](typescript-eslint/typescript-eslint@0edb94a)) ### [`v5.34.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#5340-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5331v5340-2022-08-22) [Compare Source](typescript-eslint/typescript-eslint@v5.33.1...v5.34.0) ##### Bug Fixes - **eslint-plugin:** \[no-useless-constructor] handle parameter decorator ([#5450](typescript-eslint/typescript-eslint#5450)) ([864dbcf](typescript-eslint/typescript-eslint@864dbcf)) ##### Features - **eslint-plugin:** \[prefer-optional-chain] support suggesting `!foo || !foo.bar` as a valid match for the rule ([#5266](typescript-eslint/typescript-eslint#5266)) ([aca935c](typescript-eslint/typescript-eslint@aca935c)) #### [5.33.1](typescript-eslint/typescript-eslint@v5.33.0...v5.33.1) (2022-08-15) ##### Bug Fixes - missing placeholders in violation messages for `no-unnecessary-type-constraint` and `no-unsafe-argument` (and enable `eslint-plugin/recommended` rules internally) ([#5453](typescript-eslint/typescript-eslint#5453)) ([d023910](typescript-eslint/typescript-eslint@d023910)) </details> <details> <summary>typescript-eslint/typescript-eslint (@typescript-eslint/parser)</summary> ### [`v5.35.1`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5351-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5350v5351-2022-08-24) [Compare Source](typescript-eslint/typescript-eslint@v5.35.0...v5.35.1) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) ### [`v5.35.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5350-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5340v5350-2022-08-24) [Compare Source](typescript-eslint/typescript-eslint@v5.34.0...v5.35.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) ### [`v5.34.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#5340-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5331v5340-2022-08-22) [Compare Source](typescript-eslint/typescript-eslint@v5.33.1...v5.34.0) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) #### [5.33.1](typescript-eslint/typescript-eslint@v5.33.0...v5.33.1) (2022-08-15) **Note:** Version bump only for package [@typescript-eslint/parser](https://github.com/typescript-eslint/parser) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about these updates again. --- - [x] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Co-authored-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1519 Reviewed-by: Epsilon_02 <epsilon_02@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>

feat(eslint-plugin): [explicit-member-accessibility] autofix missing …

37bfd8f

…accessibility

jtbandes changed the title ~~feat(eslint-plugin): [explicit-member-accessibility] autofix missing accessibility~~ feat(eslint-plugin): [explicit-member-accessibility] autofix missing accessibility by adding public Aug 17, 2022

bradzacher added enhancement New feature or request awaiting response Issues waiting for a reply from the OP or another party labels Aug 17, 2022

jtbandes added 2 commits August 17, 2022 18:35

Convert auto-fix to suggestions

a955225

Merge branch 'main' into fix-explicit-member-accessibility

b7a4d69

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Aug 18, 2022

JoshuaKGoldberg requested changes Aug 18, 2022

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Aug 18, 2022

jtbandes and others added 6 commits August 18, 2022 10:59

Merge remote-tracking branch 'upstream/main' into fix-explicit-member…

0d57ef5

…-accessibility

remove trim()s

5fd7ae3

Update packages/eslint-plugin/src/rules/explicit-member-accessibility.ts

d38da51

Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>

Merge remote-tracking branch 'origin/fix-explicit-member-accessibilit…

65dd4ce

…y' into fix-explicit-member-accessibility

merge message IDs

4623489

!

b46d54e

jtbandes changed the title ~~feat(eslint-plugin): [explicit-member-accessibility] autofix missing accessibility by adding public~~ feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers Aug 19, 2022

jtbandes added 2 commits August 22, 2022 09:37

Merge branch 'main' into fix-explicit-member-accessibility

143c1aa

Merge branch 'main' into fix-explicit-member-accessibility

6395b8f

JoshuaKGoldberg mentioned this pull request Aug 24, 2022

[explicit-member-accessibility] "explicit" option suggestions #4647

Closed

3 tasks

Merge branch 'main' into fix-explicit-member-accessibility

5471380

JoshuaKGoldberg approved these changes Aug 24, 2022

View reviewed changes

JoshuaKGoldberg enabled auto-merge (squash) August 24, 2022 14:35

JoshuaKGoldberg merged commit 0edb94a into typescript-eslint:main Aug 24, 2022

jtbandes deleted the fix-explicit-member-accessibility branch August 24, 2022 14:48

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

jtbandes commented Aug 17, 2022

nx-cloud bot commented Aug 17, 2022 •

edited

Loading

typescript-eslint bot commented Aug 17, 2022

netlify bot commented Aug 17, 2022 •

edited

Loading

codecov bot commented Aug 17, 2022 •

edited

Loading

bradzacher commented Aug 17, 2022 •

edited

Loading

jtbandes commented Aug 18, 2022 •

edited

Loading

jtbandes commented Aug 18, 2022

JoshuaKGoldberg left a comment

JoshuaKGoldberg Aug 18, 2022

jtbandes Aug 18, 2022

JoshuaKGoldberg Aug 18, 2022

jtbandes commented Aug 18, 2022

jtbandes commented Aug 23, 2022

JoshuaKGoldberg commented Aug 24, 2022

JoshuaKGoldberg left a comment

jtbandes commented Aug 24, 2022

bradzacher commented Aug 24, 2022

feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

feat(eslint-plugin): [explicit-member-accessibility] suggest adding explicit accessibility specifiers #5492

Conversation

jtbandes commented Aug 17, 2022

PR Checklist

Overview

nx-cloud bot commented Aug 17, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Aug 17, 2022

netlify bot commented Aug 17, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

codecov bot commented Aug 17, 2022 • edited Loading

Codecov Report

bradzacher commented Aug 17, 2022 • edited Loading

jtbandes commented Aug 18, 2022 • edited Loading

jtbandes commented Aug 18, 2022

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 18, 2022

Choose a reason for hiding this comment

jtbandes Aug 18, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg Aug 18, 2022

Choose a reason for hiding this comment

jtbandes commented Aug 18, 2022

jtbandes commented Aug 23, 2022

JoshuaKGoldberg commented Aug 24, 2022

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

jtbandes commented Aug 24, 2022

bradzacher commented Aug 24, 2022

nx-cloud bot commented Aug 17, 2022 •

edited

Loading

netlify bot commented Aug 17, 2022 •

edited

Loading

codecov bot commented Aug 17, 2022 •

edited

Loading

bradzacher commented Aug 17, 2022 •

edited

Loading

jtbandes commented Aug 18, 2022 •

edited

Loading