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

Allow member accessibility even for public #34

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Aug 28, 2023

With the scenes architecture we heavily use classes (and we use classes heavily already in core Grafana for data sources and other components).

We do allow protected and private member accessibility but not public, this to me creates unaligned/inconsistent code.

I think we should allow the public accessibility modifier. I know it is the default behavior, and it does not add any runtime behavior. It does provide great value.

It makes it super clear that the author actually thought about the intended use the function. When we only allow private/protected it's not always clear for functions that miss this keyword. Maybe the author just forgot? But a function that has the public keyword it becomes much more clear that the function is intended to be used from the outside.

It also makes code cleaner / and easier to read when all functions have modifiers:

Our current rules enforce this:

image

And does not allow this:

Here it is very clear that the intent of the author that the method is to be used from outside

image

Looking at other large typescript code bases they usually allow public keywords. The vscode code base does NOT enforce member accessibility so it's a bit inconsistent (some classes use public on all public methods some classes don't). I think that inconsistency is the only downside. The upside is that developers who want to be clear in the code they write, can and are allowed to.

Copy link
Member

@dprokop dprokop left a comment

Choose a reason for hiding this comment

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

Agree with the argumentation. Wonder why we even threw an error for the public modifier.

Think as long as this change ain't enforced and does not cause need to update million classes, this will not ham us.

Copy link

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

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

indifferent to using public (I personally wouldn't), but I dislike lint rules that enforce things that don't actually matter.

@torkelo torkelo merged commit 97dfd6a into master Aug 28, 2023
1 check passed
@dprokop
Copy link
Member

dprokop commented Aug 28, 2023

but I dislike lint rules that enforce things that don't actually matter

well said @joshhunt

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.

None yet

3 participants