Thanks for the PR, @eranshabi!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint

How is this different from "@typescript-eslint/typedef" + "@typescript-eslint/explicit-function-return-type"?

How is this different from "@typescript-eslint/typedef" + "@typescript-eslint/explicit-function-return-type"?

This rule is for public methods. Public methods are meant to be used in scopes external to the class itself, and having them typed makes them more readable and easier to use. Sometimes you don't want to type everything but only what you think is important.

Also this rule has the option to ignore specific methods which you don't care to type (in our case we use it for react lifecycle hooks).

Put another way, the intention here is to provide a way to enforce explicit typing of module boundaries so that you can be sure of the exact types you're exposing.

In the same vein as @octogonz's question - would this be better served as an option on explicit-function-return-type, instead of an entirely new rule?
I like the idea, for sure, but it seems like a user would use either this rule, or e-f-r-t, as this rule is a subset of e-f-r-t.

Thanks for the feedback.
I have no problem merging this rule with another, yet I still think we don't fully understand each other - This rule checks both the return type and the parameters type of all public class methods, so it's not a subset of another rule afaik.

This rule checks both the return type and the parameters type of all public class methods

I realised that a little after I wrote the comment (but I wasn't at my keyboard), sorry.

It is probably worth keeping this rule as is. In future we can augment it (and rename it) so that it supports more cases of explicitly typing module boundaries.

BTW it's worth pointing out that TSLint's typedef handled all the different kinds of type annotations (parameters, return values, properties, variables, etc). I always found a little strange for ESLint to treat them as separate rules.

If we followed TSLint's model, then "don't require types for private members" could be an option for the one rule, rather than having these criteria split across 3 rules. For the end user experience, it might make configuration easier, since people wouldn't have to understand the exclusive-or between typedef + explicit-function-return-type versus no-untyped-public-signature. Just a thought.

The TL;DR is that nobody wanted to implement the full typedef rule until very recently, but efrt was wanted much earlier.

Most of typedef is covered by noImplicitAny (esp in the latest version), and there are a number of rare use cases (like explicitly typing destructuring), so nobody saw the need in implementing the entire rule.

But ~2 years ago someone saw value in enforcing efrt, and so that got implemented.
Since then efrt has grown larger than what typedef supports. No doubt it will probably continue evolve as people want more cases / flexibility (eg I've been thinking about making it more accurate by making it "type-aware").

I tend to want to avoid monolithic rules as well. It sounds great being able to configure a single rule to do all of these things, but users often avoid the rules because they're big and scary (documentation is long, and there are many options to configure), and maintaining the rule can be hard because you're putting X distinct code paths into a single file.

There's tradeoffs to both approaches.

so if I understand you correctly @bradzacher, this can be merged?
If not, what changes are required?

I haven't had a chance to review the code yet, sorry.
I'll be sure to comment with required changes (if any) once I've had a chance to review

fixed the build 🙃

@bradzacher please?

This PR is in the queue for me to review.
I haven't had any spare time over the last few weeks due to family things.
I'll get to it soon.

Codecov Report

Merging #801 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #801      +/-   ##
==========================================
+ Coverage   94.02%   94.05%   +0.03%     
==========================================
  Files         122      123       +1     
  Lines        5253     5283      +30     
  Branches     1456     1468      +12     
==========================================
+ Hits         4939     4969      +30     
  Misses        179      179              
  Partials      135      135

Your new change @bradzacher breaks consctuctor description:

  public constructor() {
    this.httpLogger = loggerFactory.getLoggerColor("http", "#680061");
  }

16:3 warning Public method has no return type @typescript-eslint/no-untyped-public-signature

please raise a new issue for this.
Comments on closed PRs is not a great way to report issues or track work.