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

fix(eslint-plugin): [explicit-member-accessibility] fix parameter properties bugs #912

Merged

Conversation

ankeetmaini
Copy link
Contributor

@ankeetmaini ankeetmaini commented Aug 27, 2019

Fixes #617
Fixes #857

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @ankeetmaini!

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

@ankeetmaini
Copy link
Contributor Author

This fixes most of the cases, there's just one gotcha, that parameter prop check will be skipped if constructors: off

@codecov
Copy link

codecov bot commented Aug 27, 2019

Codecov Report

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

@@            Coverage Diff             @@
##           master     #912      +/-   ##
==========================================
+ Coverage   94.05%   94.06%   +<.01%     
==========================================
  Files         113      113              
  Lines        4931     4937       +6     
  Branches     1376     1378       +2     
==========================================
+ Hits         4638     4644       +6     
  Misses        166      166              
  Partials      127      127
Impacted Files Coverage Δ
...-plugin/src/rules/explicit-member-accessibility.ts 90.74% <100%> (+1.15%) ⬆️

@bradzacher bradzacher added breaking change This change will require a new major version to be released enhancement New feature or request bug Something isn't working and removed enhancement New feature or request labels Aug 27, 2019
@ankeetmaini
Copy link
Contributor Author

I am sorry, this is not a breaking change, as it doesn't change the existing config.

@bradzacher bradzacher removed the breaking change This change will require a new major version to be released label Aug 27, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

two comments, otherwise LGTM.
Thanks for taking the time to do this, and putting up with the churn!

break;
}
case 'no-public': {
if (node.accessibility === 'public') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this should also check for readonly.
public foo is valid (otherwise it's not a parameter property), but public readonly foo is not

Suggested change
if (node.accessibility === 'public') {
if (node.accessibility === 'public' && node.readonly) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked public readonly foo in ASTExplorer using @typescript-eslint/parser, it showed that it is a valid AST.

https://astexplorer.net/#/gist/462492e71de4506a8ece6dbd055d2196/4273aa53fae3642835f358371cda2fe08e350bed

Also if I were to add that check, if parameterProperties='no-public', then public foo: string will pass as readonly is missing but still the access modifier is public.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For explicit:
public foo and public readonly foo are both considered valid (obviously!)

For no-public:
public foo is considered valid, because without public, it's not a parameter prop.

public readonly foo is considered invalid, because you can remove the accessibility modifier, and it's still a parameter property: readonly foo.


We should probably document this in the readme so that it's a bit clearer, as I don't think that this is entirely intuitive.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've incorporated these changes, thanks a lot for putting these down. HAve added in the readme too :)

Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for doing this.

@bradzacher bradzacher merged commit ccb98d8 into typescript-eslint:master Sep 2, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
2 participants