[class-methods-use-this] Allow when required by interface

[BBosman]

Repro

I'd like to request a @typescript/class-methods-use-this version of class-methods-use-this, which takes interfaces into account. If an interface requires a (public) method to be there, then the rule should allow it, even though it's not actually referencing this.

{
  "rules": {
    "class-methods-use-this": "error"
  }
}

interface IStuff {
  doSomething(): void;
}

class Stuff implements IStuff {
  public doSomething(): void {
    // Do something without touching `this`.
  }
}

Expected Result
This to not flag class-methods-use-this.

Actual Result
Getting a class-methods-use-this violation on the doSomething method.

Additional Info

If this is impossible to do, I would also be happy with an allowPublic setting for that rule, that just skips checking public methods for usage of this.

Versions

package	version
@typescript-eslint/eslint-plugin	2.4.0
@typescript-eslint/parser	2.4.0
TypeScript	3.6.4
ESLint	6.5.1
node	10.15.0
npm	6.12.0

[bradzacher]

How come you want to use this rule?
IMO there's little value in it, so I'm curious on what your reasons are for using it.

As an aside - if you change it from a method to a property (public doSomething = (): void => {), then it will no longer match the rule, because the base rule doesn't support class properties (a bit of a hack)

[BBosman]

Mostly for testability. Methods that don't use this are often an indication of it being a utility function. Making it a (public) static easily allows for writing unit tests for it, increasing coverage.

[Robbos]

Like @BBosman, I also find it strange that my implementations of interface methods get an error on the class-methods-use-this rule.

I wanted to use an interface so that I can plug in behaviour with specific implementation classes. However, these classes don't need this.
Doesn't this feature request make sense then?
If not, should I use a different approach and if so, which one?

[bradzacher]

As with anything in this repo, unless we (the maintainers) have said it's not a good fit, then it's welcome to be PR'd.

To me though, it seems like a bit of a code smell, or a linting config smell.
If you're creating interfaces and regularly implementing those methods without using this, then it seems like either you're purposely looking to circumvent your lint rule, or that this rule is not a good fit for your codebase.

I'd lean toward the latter - you're purposely using a coding style which causes a lint rule to fire, so the lint rule is probably a bad fit.

[devsone]

I think what suggested by @BBosman is actually required.

• on inheritance situation when a child class needs to implement an abstract method or override a method

• when a class implements an interface and it is required to implement a method

for both situations, the rule class-methods-use-this generates errors if there is no way for you to reference this on your code. on the other hand because of inheritance or implementation, it is not possible to make it static. Considering using an external library, it is not possible to change the behavior of parent.

I think ignoring these situations should be the default behavior of this rule but right now is it possible to extend this rule and add options for ignoring interface/inheritance situations?? It's obvious we need it bad!

[6XGate]

I would like to bring up that many OOP paradigms using interfaces or overridden methods will result in methods that don't need access to an instance state, but must be an instance method. A good example would be used in an IoC container. Say the container is expected to have an implementation of a Logger contract. That default implementation could simply send to the console host object which does not require holding a state of any kind on the contract's implementation. If this was replaced with a file logger it would require a state, but not the default. The default case would never use a this reference anywhere.

I would think the default behavior should be to ignore methods that implement an interface method or overridden a base class method.

[stefan-schweiger]

This is definitely missed. When using Angular there are many interfaces your components or classes might implement, but it often is not really required to reference this. For example very common might be a pipe which does some calculation or string manipulation without the need for anything else.

Straight from the angular example code:

import { Pipe, PipeTransform } from '@angular/core';
/*
 * Raise the value exponentially
 * Takes an exponent argument that defaults to 1.
 * Usage:
 *   value | exponentialStrength:exponent
 * Example:
 *   {{ 2 | exponentialStrength:10 }}
 *   formats to: 1024
*/
@Pipe({name: 'exponentialStrength'})
export class ExponentialStrengthPipe implements PipeTransform {
  transform(value: number, exponent?: number): number {
    return Math.pow(value, isNaN(exponent) ? 1 : exponent);
  }
}

You don't have the choice to implement this kind of method in a static way even though it never uses this.

[pixelbucket-dev]

We have the same issue when using class decorators in StencilJS. Most of the time in this methods we don't use this but also cannot just move the method into a static function because of the decorator.

A fix could be either one of these:

1. a further field in Exceptions called exceptDecorators
2. in Exceptions the field exceptMethods should allow regex glob patterns to filter out methods that follow a certain naming pattern, e.g. every method that starts with handle… will not be caught by this rule.
   "class-methods-use-this": ["error", { "exceptMethods": ["handle*"] }]

[stefan-schweiger]

@bradzacher In the near future Typescript will have support for the override keyword. Can we at least get an option to disable the error when override is used?

[luxaritas]

Related: #52

[bradzacher]

Please don't @ tag maintainers, as it just creates notification spam for the volunteer maintainers.

We are a community run project. The volunteer maintainers spend most of their time triaging issues and reviewing PRs. This means that most issues will not progress unless a member of the community steps up and champions it.

If this issue is important to you - consider being that champion.
If not - please just subscribe to the issue and wait patiently for someone else to champion it.

[stefan-schweiger]

@bradzacher fair enough that the community can't expect new features to appear out of thin air. But you were active in this discussion and asked questions and objected to answers people made.

So since this is still a discussion and the rest of the community can't know who unsubscribed from an issue or not I think it's not wrong to tag people who participated in this issue...