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

adds warning for mismatched class member accessibility #402

Merged
merged 2 commits into from
May 4, 2021

Conversation

georgejecook
Copy link
Contributor

No description provided.

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Looks good. I asked for a few small changes.

Also, the github actions build is failing right now because coveralls (our coverage tool) has a server-side error. I'll keep an eye on that.

src/DiagnosticMessages.ts Outdated Show resolved Hide resolved
//child member has different visiblity
if (
//is a method
isClassMethodStatement(member) &&
Copy link
Member

Choose a reason for hiding this comment

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

Why only class methods? Fields should also be included in this. TypeScript applies this restriction to both methods and fields.

Copy link
Contributor Author

@georgejecook georgejecook Apr 28, 2021

Choose a reason for hiding this comment

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

I did this as we don't support overriding field yet, do we? I don't want to have to implement that as part of this fix, as that's a new feature.
image

@TwitchBronBron TwitchBronBron merged commit cf19c70 into master May 4, 2021
@TwitchBronBron TwitchBronBron deleted the add-warning-for-mismatch-class-visbility branch May 4, 2021 02:47
@TwitchBronBron
Copy link
Member

Looks good. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants