Update: Improve array-callback-return report message #13395

mrflip · 2020-06-05T19:37:18Z

What changes did you make? (Give an overview)

The array-callback-return rule should explain that a return value is required because the surrounding method requires it. This makes it clear that eg. .filter() expects the passed function to return something, rather than a general expectation of that function.

The error messages for expectedAtEnd/Inside now read Method .{{arrayMethodName}}() expected a value to be returned at the end of {{name}}. and Method .{{arrayMethodName}}() expected a return value from {{name}}.

Prerequisites checklist

[Y] I have read the contributing guidelines.
[Y] II signed the CLA

What is the purpose of this pull request? (put an "X" next to an item)

[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[X] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What rule do you want to change?: array-callback-return
Does this change cause the rule to produce more or fewer warnings?: same
How will the change be implemented? (New option, new default behavior, etc.)?: new error messages
Please provide some example code that this change will affect:

    Object.entries(extraParams.values).map(([op, argStrs]) => {
      ops[op].extraParams = Object.fromEntries(argStrs.map((pair) => pair.split(/: */)))
    })

What does the rule currently do for this code?: It says "Expected to return a value in arrow function". What? Who said arrow functions have to return? I do that all the time and there's no error.
What will the rule do after it's changed?: It will say "Method .map() expected a return value from arrow function". Aha! I need to change .map() to .forEach().
I Read the pull request guide (https://eslint.org/docs/developer-guide/contributing/pull-requests)
I modified the tests for this change
I think it automatically updates documentation for this change?

Is there anything you'd like reviewers to focus on?

If I were writing the test I would say 'expects' and not 'expected'; is the past tense a style guide point or an accident of history? nbd.

kaicataldo

I support this change - one small request to change to the error message for clarity.

lib/rules/array-callback-return.js

mrflip

Great! Fixed the tests to pass with the new message.

kaicataldo · 2020-06-25T18:01:09Z

@eslint/eslint-team Can we get a few more eyes on this? Looks like we need a champion and one more 👍 to accept this.

mdjermanovic · 2020-06-25T21:55:34Z

Makes sense to me, too. I'll champion this.

If this gets accepted, could we also fix the message for .forEach?

Also, Array.from isn't Array.prototype.from.

I think it automatically updates documentation for this change?

No, but I don't think there is something in the docs that should be updated for this particular change.

If I were writing the test I would say 'expects' and not 'expected'; is the past tense a style guide point or an accident of history?

Agreed to change to "expects".

The array-callback-return rule should explain that a return value is required because the surrounding method requires it. This makes it clear that eg. .filter() expects the passed function to return something, rather than a general expectation of that function. The error messages for expectedAtEnd/Inside now read 'Method .{{arrayMethodName}}() expected a value to be returned at the end of {{name}}.' and 'Method .{{arrayMethodName}}() expected a return value from {{name}}.'

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>

* Made tests match proposed new message.

* forEach has a message, and describes why no return value is expected. * present tense ('Array.from expects no' vs 'Array.from expected no') * Added explicit check of the forEach message

mrflip · 2020-06-28T01:54:36Z

I think this addresses all review points:

Rebased to master
forEach has a message, and describes why no return value is expected.
Made the messages present tense ('Array.from expects no…' vs 'Array.from expected no…')
Attributed methods correctly: Array.from, Array.prototype.(everything else)
Added explicit check of the forEach message

@kaicataldo Could you re-review and let me know if all changes have been taken care of?

mdjermanovic

Looks good in general, there's one thing that should be fixed if this change gets accepted (it still needs one more 👍 from a team member).

mdjermanovic · 2020-06-28T12:42:57Z

lib/rules/array-callback-return.js

@@ -273,7 +273,8 @@ module.exports = {
                        node,
                        messageId,
                        data: {
-                            name: lodash.upperFirst(astUtils.getFunctionNameWithKind(funcInfo.node))
+                            name: astUtils.getFunctionNameWithKind(funcInfo.node),
+                            arrayMethodName: "Array.prototype.".concat(funcInfo.arrayMethodName)


This path can also report Array.from, e.g.:

/* eslint array-callback-return: error */ // it shouldn't be "Array.prototype.from() expects a return value from arrow function" Array.from(foo, () => { return; });

Maybe we could make a function that gets funcInfo.arrayMethodName and returns its "full name", and then use that function here and from the other context.report?

Thanks, those lines felt fishy and a function is the right answer. I went ahead and included all class methods (of, isArray, from) in case the function was ever lifted somewhere else. Pushed a commit with this change.

mrflip

Pushed a commit to clean up the fully-qualified method naming

mrflip · 2020-06-28T13:12:00Z

lib/rules/array-callback-return.js

@@ -273,7 +273,8 @@ module.exports = {
                        node,
                        messageId,
                        data: {
-                            name: lodash.upperFirst(astUtils.getFunctionNameWithKind(funcInfo.node))
+                            name: astUtils.getFunctionNameWithKind(funcInfo.node),
+                            arrayMethodName: "Array.prototype.".concat(funcInfo.arrayMethodName)


Thanks, those lines felt fishy and a function is the right answer. I went ahead and included all class methods (of, isArray, from) in case the function was ever lifted somewhere else. Pushed a commit with this change.

kaicataldo · 2020-06-29T23:50:22Z

@eslint/eslint-team We just need one more 👍 to accept this.

mdjermanovic

LGTM, thanks!

btmills

Added my 👍 and marking as accepted. LGTM, thanks for this nice improvement @mrflip!

eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Jun 5, 2020

kaicataldo suggested changes Jun 13, 2020

View reviewed changes

lib/rules/array-callback-return.js Outdated Show resolved Hide resolved

lib/rules/array-callback-return.js Outdated Show resolved Hide resolved

mrflip commented Jun 15, 2020

View reviewed changes

mrflip requested a review from kaicataldo June 16, 2020 03:59

mdjermanovic self-assigned this Jun 25, 2020

mrflip and others added 4 commits June 27, 2020 19:35

Update lib/rules/array-callback-return.js

e6f640f

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>

Update lib/rules/array-callback-return.js

65374e1

Co-authored-by: Kai Cataldo <kai@kaicataldo.com>

Update: Improve array-callback-return report (explains why) patch

0cdf1a8

* Made tests match proposed new message.

mrflip force-pushed the ArrayCbReturnMessage branch from 0da1aab to 0cdf1a8 Compare June 28, 2020 00:36

fix: forEach message, .from Owner

71847d4

* forEach has a message, and describes why no return value is expected. * present tense ('Array.from expects no' vs 'Array.from expected no') * Added explicit check of the forEach message

mdjermanovic requested changes Jun 28, 2020

View reviewed changes

fix: cleaner production of full-qyalified method name

2e349c2

mrflip commented Jun 28, 2020

View reviewed changes

mdjermanovic changed the title ~~Update: Improve array-callback-return report (explains why)~~ Update: Improve array-callback-return report message Jun 30, 2020

mdjermanovic approved these changes Jun 30, 2020

View reviewed changes

btmills approved these changes Jun 30, 2020

View reviewed changes

btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jun 30, 2020

kaicataldo approved these changes Jun 30, 2020

View reviewed changes

kaicataldo merged commit ff5317e into eslint:master Jul 3, 2020

eslint-deprecated bot locked and limited conversation to collaborators Dec 31, 2020

eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: Improve array-callback-return report message #13395

Update: Improve array-callback-return report message #13395

mrflip commented Jun 5, 2020

kaicataldo left a comment

mrflip left a comment

kaicataldo commented Jun 25, 2020

mdjermanovic commented Jun 25, 2020

mrflip commented Jun 28, 2020

mdjermanovic left a comment

mdjermanovic Jun 28, 2020

mrflip Jun 28, 2020

mrflip left a comment

mrflip Jun 28, 2020

kaicataldo commented Jun 29, 2020

mdjermanovic left a comment

btmills left a comment

Update: Improve array-callback-return report message #13395

Update: Improve array-callback-return report message #13395

Conversation

mrflip commented Jun 5, 2020

What changes did you make? (Give an overview)

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

Is there anything you'd like reviewers to focus on?

kaicataldo left a comment

Choose a reason for hiding this comment

mrflip left a comment

Choose a reason for hiding this comment

kaicataldo commented Jun 25, 2020

mdjermanovic commented Jun 25, 2020

mrflip commented Jun 28, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

mdjermanovic Jun 28, 2020

Choose a reason for hiding this comment

mrflip Jun 28, 2020

Choose a reason for hiding this comment

mrflip left a comment

Choose a reason for hiding this comment

mrflip Jun 28, 2020

Choose a reason for hiding this comment

kaicataldo commented Jun 29, 2020

mdjermanovic left a comment

Choose a reason for hiding this comment

btmills left a comment

Choose a reason for hiding this comment