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

[explicit-member-accessibility] Remove rule #111

Closed
swansontec opened this issue Aug 7, 2019 · 7 comments · Fixed by #124
Closed

[explicit-member-accessibility] Remove rule #111

swansontec opened this issue Aug 7, 2019 · 7 comments · Fixed by #124

Comments

@swansontec
Copy link
Contributor

swansontec commented Aug 7, 2019

This rule does not have an automatic fix, so applying it to a new codebase requires large amounts of manual intervention.

Plus, it goes against the Standard.js style. All class members are public by default, so there is no reason to write this explicitly. Standard.js tends to remove things that the language infers automatically like semicolons or useless constructors, so requiring public everywhere seems counter to this philosophy. It also makes the code look less Javascript-ish, since plain ES6 classes doesn't even have accessibility declarations.

@swansontec swansontec changed the title [explicit-member-accessibility] Remove / loosen rule [explicit-member-accessibility] Remove rule Aug 7, 2019
@mightyiam
Copy link
Owner

I agree.

The benefit that I found is that it compels the writer to decide the accessibility for each member. And where members would be "neglected" as default, the writer would likely use more restrictive specifiers on some of them, when "forced" to.

Yes, it does go a long way from JS, but this is TypeScript and here's an opportunity to decide on good practices.

Migrating to a rule set usually involves some manual intervention. This can have a large impact, yes. Is it worth it? Does it improve the code? I feel that it does. And how many lines would break in a project? I haven't done any class based development in TypeScript, so I'm not experienced enough to understand the weight of this rule.

@LinusU?

@LinusU
Copy link
Contributor

LinusU commented Aug 13, 2019

Standard.js tends to remove things that the language infers automatically like semicolons or useless constructors, so requiring public everywhere seems counter to this philosophy.

I kind of feel like this as well. In face, I think that I would prefer a rule that forced you to not have public since that is the default.

@mightyiam
Copy link
Owner

And the benefit that it compels you to decide on member accessibility—what about that? Doesn't that offer value? Wouldn't it result in your code ending up more reliable, because you set some members as not public? Wouldn't it be prudent of as as developers to use all of the means available to better our code? Wouldn't this rule be one such mean? Yet, if it is so bothersome that it is impractical, it should not be in Standard. I understand that.

@swansontec
Copy link
Contributor Author

And the benefit that it compels you to decide on member accessibility—what about that?

That would be like saying we should use semicolons, because they "compel you to decide where lines end". Standard.js users don't need semicolons, though - we quickly learned the implicit line-ending rules when we started. After that, line ending is natural and obvious, and semicolons are just extra noise.

Likewise, members are public unless you say otherwise. Omitting the accessibility specifier is making a clear choice - it's a choice for public. Typing redundant keywords offers no real benefit once you get used to the language rules.

@mightyiam
Copy link
Owner

With line endings, there is no decision to be made. You only ever mean one thing. You always know where you mean to end the line. There is no decision to make.

Member accessibility is different, because it's about making a decision regarding the accessibility of a member of a class. There is a decision to make.

So the comparison between the two above as a response to the benefit of the rule forcing a decision is irrelevant, because in ASI there is no decision to be made.

In member accessibility, forcing a decision by forcing to explicitly define, does force a decision, and that means that the developer was forced to make that decision. That feels like a benefit to me, as it could produce better code, as private and protected increase the type safety of the code. Let's take at the very least the example of the developer who doesn't even know about the accessibility modifiers—wouldn't it have educational and code result benefit in his case?

I'm not saying it's clearly worth the extra work, though. I'm quite thorough and so I'd probably go along with it, but this is a public library and so I'd like to get some more feelings here than me and you two. And I'd like you to address this, please. Even if just by stating that you feel it's not worth it.

@swansontec
Copy link
Contributor Author

swansontec commented Aug 15, 2019

Let's take at the very least the example of the developer who doesn't even know about the accessibility modifiers—wouldn't it have educational and code result benefit in his case?

Ah, I see what you are saying. I usually assume that developers know the rules and use them to their advantage. An experienced developer will naturally use private and protected everywhere possible, since stable public API's require a lot of extra work. Meanwhile, a truly bad developer will just type public everywhere because that's the fastest way to "make the errors go away."

You aren't worried about these two groups. Instead, you want to encourage good habits in more mid-level developers. I see how this rule could have benefits for that group.

I'm not saying it's clearly worth the extra work, though. [...] And I'd like you to address this, please. Even if just by stating that you feel it's not worth it.

I could live with this rule if it were auto-fixable, but it's not. My day job is a React Native project with several hundred classes and a few thousand members, so fixing things manually would be quite expensive. Other projects moving to Standard.js + TypeScript would have the same problem.

The Standard.js change log gives an estimate of how many projects each new rule would affect, based on the official test suite. I don't think the test suite contains any TypeScript projects, though. If it did, my guess is that this rule would affect close to 100% of projects that aren't already using it. It's a very common pattern.

Between the manual fix and broad reach, I don't think this rule is worth it. If a bunch of people feel strongly that this rule should stay, making it auto-fixable should be a high priority.

@mightyiam
Copy link
Owner

Thank you for your input @swansontec. I truly appreciate it.

I can imagine this being auto-fixable. Yet, with some caveats. Expecially regarding importable projects, where it could not be analyzed whether the member is meant to be publicly used or not.

Let's remove the rule.

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 a pull request may close this issue.

3 participants