feat(eslint-plugin): [explicit-function-return-type] allowHigherOrderFunctions (#193)

[Svish]

The last comment of #193 said that you were Accepting PRs from the community (we love new contributors!!), and I really wanted #193 implemented, so... I gave it a shot, and... it seems to work? All the existing tests and the 3 new ones I wrote for this seems to run fine at least? 🤔 🎉

Fixes #193

[codecov]

Codecov Report

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

@@            Coverage Diff             @@
##           master     #538      +/-   ##
==========================================
+ Coverage   94.23%   94.27%   +0.03%     
==========================================
  Files         104      104              
  Lines        4234     4246      +12     
  Branches     1152     1160       +8     
==========================================
+ Hits         3990     4003      +13     
  Misses        142      142              
+ Partials      102      101       -1

Impacted Files	Coverage Δ
...-plugin/src/rules/explicit-function-return-type.ts	100% <100%> (+2.7%)	⬆️

[JamesHenry]

On mobile, so can't write much detail write now, but I don't think this should go in as is.

Currying is not specific to arrow functions, so it is confusing to have this name/relationship IMO

[Svish]

Good point. Do you have some examples of what else should pass/fail with this new option?

[bradzacher]

The only case you've handled is an arrow function which returns an arrow function and has no body

const x = () => () => 1

Should probably support all of these cases, as no doubt people will ask for them down the line.

function FunctionDeclaration() {
  return function FunctionExpression_Within_FunctionDeclaration() {
    return function FunctionExpression_Within_FunctionExpression() {
      return () => { // ArrowFunctionExpression_Within_FunctionExpression
        return () => // ArrowFunctionExpression_Within_ArrowFunctionExpression
          () => 1 // ArrowFunctionExpression_Within_ArrowFunctionExpression_WithNoBody
      }
    }
  }
}

function FunctionDeclaration() {
  return () => { // ArrowFunctionExpression_Within_FunctionDeclaration
    // etc
  }
}

I.e. every single combination of FunctionExpression / FunctionDeclaration / ArrowFunctionExpression

[Svish]

@JamesHenry Thanks to the examples provided by @bradzacher and some time looking at them through https://astexplorer.net, I think I figured out how to implement it for all(?) cases now (i.e. not just for ArrowFunctionExpression). The tests seem to run fine at least. Branch updated. 🙂👍

stale

[bradzacher]

Code + Tests LGTM!

One last change I can see: Please document a few more cases in the rule rule doc just to clarify it works for non-arrow functions too.

[Svish]

@bradzacher I see you already added an approval, but just so it's said: Docs have been updated with function example, and I added two tests to cover a missing branch in my changes.

I also looked into adding a test to cover the very last uncovered branch in the file, but I'm pretty sure it's actually impossible to hit that branch?

We only get to the if when node is either FunctionExpression or ArrowFunctionExpression, which seems to always have a parent of ExpressionStatement? And even if that were to not be the case, there should still be a parent of Program?

So, to get the coverage up to 100% (and for others not to waste time trying to cover that branch too) I added an istanbul ignore else there.

[JamesHenry]

I know I'm being the naming police on this, but this still isn't really currying that allowCurrying is allowing.

The implementation is more broad than that, it is allowing any higher order function pattern.

I think that is probably what is most valuable anyway, so it's just the name that needs to change, so that users aren't confused and think it checks explicitly for currying patterns.

I'm not married to it, but allowHigherOrderFunctions ?

[Svish]

@JamesHenry Could you explain what the difference is? From what I can see, currying looks like what this option allows. I'm reading about higher order functions, but they seem a lot more broader, like taking functions as arguments as well, which has nothing to do with this. There seems to be another concept called partial application, which to me looks like a slightly broader currying, but yeah... not really knowledgeable in this area...

I don't mind changing the name though. What do you think @bradzacher?

[bradzacher]

IIRC technically currying is more the concept of functions which automatically return functions if you don't pass all of the required arguments, instead of explicitly defined functions returning functions.

In that regard, calling these higher order functions is technically more correct.

I think that either will work - people will probably understand either way.
But in case they don't, probs best to change it to something like allowHigherOrderFunctions.

[Svish]

@JamesHenry @bradzacher Alrighty, the option has now been renamed to technically more correct allowHigherOrderFunctions, and documentation and code adjusted accordingly. 🙂

[Svish]

@JamesHenry Is it good now? Anything else preventing a ✔ from you as well? Would really like to get this PR merged in, closed and released so I can make use of this new option in our project. 🙂

[Svish]

I know I'm being the naming police on this, but this still isn't really currying that allowCurrying is allowing.

The implementation is more broad than that, it is allowing any higher order function pattern.

I think that is probably what is most valuable anyway, so it's just the name that needs to change, so that users aren't confused and think it checks explicitly for currying patterns.

I'm not married to it, but allowHigherOrderFunctions ?

@JamesHenry Your requested change was fixed over a week ago now. Is there still more you'd like to be changed, or is it good now?