Tslint: [Rule suggestion] Prefer or disallow get/set accessors

Created on 18 Oct 2017  路  12Comments  路  Source: palantir/tslint

A rule which either disallows get/set accessors, or requires them when possible.

Enforces this

getSomething() {
    return this.aProp * this.cProp;
}

setSomething(a: string) {
    this.bProp = a + '!!!';
}

or this

get something() {
    return this.aProp * this.cProp;
}

set aDiffThing(a: string) {
    this.bProp = a + '!!!';
}

Accessors, no accessors--I don't have a preference, really. Just looking for a rule to help w/ consistency on this matter.

Declined Rule Suggestion

Most helpful comment

+1 I'm interested in banning accessors, get/set are a little too magic for my taste

All 12 comments

You want the rule to complain only when both get and set for the same property are implemented or if any of the two are present?

Some other things to keep in mind:
get prop() can not have parameters or type parameters. So getProp(foo: string) is not affected by the rule. Similar for the setter.
The types of the get and set accessor need to match. To check that, you would need type information.
What about the following:

public foo: string;
public getFoo() { // when converted to a get accessor it would have the same name as the property
    return this.foo;
}

The easy fix would be a rule to simply ban accessors.

Another thing that comes to mind are write only properties, i.e. when only a set accessor is available. TypeScript has no special handling for that case. It will not complain if you read the property. It will just yield undefined.

I see your point about the duplicate identifiers... Not sure how to handle that.

To clarify, I imagined this rule would enforce a get accessor if the method accepts no args and returns something--anything. That's a getter, in my mind. If a method accepts one arg and sets the value of exactly one class member, it should be transformed into a set accessor.

I think the option to ban accessors altogether would be sufficient. It suits my purposes, anyway.

[EDIT] Removed stupid question.

+1 I'm interested in banning accessors, get/set are a little too magic for my taste

@aervin could you reopen this issue?

I don't think this suggestion stupid and want this rule.

While the two examples mentioned at the beginning are not exactly same, it is possible to use in same way. In general it's a good idea to restrict how to code especially when coding in team.

In my case get/set accessors caused a problem when testing by a library that doesn't support them then needed to use getXxx/setXxx. This rule helps not to keep in mind library's behavior.
https://medium.com/@TakuyaHARA/you-cant-spy-set-get-by-sinon-with-typescript-851f4a94b1e1

We've a rule which disallows get/set. If you wish, I believe we can share it for TSLint. @JoshuaKGoldberg what do you think? Or I need to create another issue for that?

@timocov I think that PR would address this issue, so IMO it makes sense to mark it as fixing this issue.

But: we should note that any rule to fix this issue should be configured to prefer either way, not hardcoded to just one.

Apologies for the late feedback on this, but I feel like this rule is a little too opinionated / complex / nuanced to belong in core TSLint. I can see in the PR that it's getting nontrivial to configure, and I'm sure many more edge cases will arise. I would rather this rule live outside this repo.

@adidahiya no problem, but the original rule was not about preferring properties or get/set methods, it was about just disallowing properties and I'm not sure that we can merge that PR with requirements for this issue. I would say it's about the same as many other no-* rules.

@adidahiya what do you think about a rule for "just disallowing properties"? Not about forcing get/set methods or JS properties.

@timocov why? that also feels too opinionated / niche. I would like to hear more support for such a rule before adding it to the core rules

@adidahiya get/set might be confused for users of that property, because you might don't expect that it's a getter _function_ and it isn't just getting the value from object - the function _may_ does a lot of thing, not just return the value.

I'm talking about restricting getters/setters, not about a preferred way to declare properties.

I would like to hear more support for such a rule before adding it to the core rules

Maybe I need to create another issue for that to wait more feedback from other users?

Was this page helpful?
0 / 5 - 0 ratings