-
-
Notifications
You must be signed in to change notification settings - Fork 69
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
Comments
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. |
I kind of feel like this as well. In face, I think that I would prefer a rule that forced you to not have |
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. |
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 |
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 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. |
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 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 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. |
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. |
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 requiringpublic
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.The text was updated successfully, but these errors were encountered: