Add allowSimpleOperations option to no-array-reduce rule

[manovotny]

This is my first time contributing something to this project besides from minor documentation tweaks, so don't feel bad telling me I did something incorrectly.

I took a stab at the open issue to allow simple sums when using reduce. I am not married to the option name, so please offer better suggestions if you have them.

Fixes #1348

[sindresorhus]

I think it should be true by default. This is the one case where "reduce" 100% makes sense.

[manovotny]

@sindresorhus I am totally open to that and can start whipping up changes. I figured it was best to start it off as a non-breaking change until a conversation could be had.

[manovotny]

@sindresorhus with the flipping of default to true it uncovered a use case that I didn't have right originally and could use your advice of how you want to handle it.

It's this from MDN's reduce documentation on parameters (emphasis added):

A value to use as the first argument to the first call of the callbackFn. If no initialValue is supplied, the first element in the array will be used as the initial accumulator value and skipped as currentValue.

For example:

const nums = [1, 2, 3, 4, 5];
console.log(nums.reduce((total, item) => total + item, 0)); // -> 15
console.log(nums.reduce((total, item) => total + item)); // -> 15

I am unsure of how to check the assumed values that are being iterated on without the ability to check initialValue.

Any great ideas? Or should the rule only work / pass when initialValue is present, thus forcing people to use an initialValue to get started?

[sindresorhus]

Or should the rule only work / pass when initialValue is present, thus forcing people to use an initialValue to get started?

I think an initial value helps readability, so I agree we can just enforce that if they want it to pass.

[manovotny]

@sindresorhus I've changed the default value to true and updated the documentation. Please let me know if there are any other changes you'd like me to make.

[fisker]

Do you think the following cases are simple enough to ignore?

const getValueByPath = (object, path) =>
	path.split('.').reduce((value, key) => value[key], object);

const queue = steps =>
	steps.reduce((promise, step) => promise.then(step), Promise.resolve());

[sindresorhus]

Do you think the following cases are simple enough to ignore?

IMHO, no.

[fisker]

I think we should also check the callback function body, only allow simple + or * (maybe ** too?). And let's remove this option, just allow these cases. WDYT?

[fisker]

We can also change these two lines in our codebase back

https://github.com/sindresorhus/eslint-plugin-unicorn/pull/704/files#diff-2c0df1d17a5ab16085062d0ff80023998f8e23ef6d48af8bc8bb14a35e569dadL4

And

https://github.com/sindresorhus/eslint-plugin-unicorn/pull/704/files#diff-962f4629ce8856ba31836e3e55a7b5f5b2d6ad52cc7fe5dcdeecb05d025b3ba4L32

[sindresorhus]

only allow simple + or * (maybe ** too?)

Yeah, and - and /.

[manovotny]

@sindresorhus @fisker both your comments make sense and seem doable. I was in all day meetings today and will be again tomorrow. I should be able to circle back to this on Friday or into the weekend. Thanks for the feedback!

[manovotny]

I've updated the PR to allow a direct BinaryExpression as the body of the function. That way we shouldn't need to check for specific operators. Let me know if I am not thinking of something by allowing this.

I also renamed the lint rule option to allowSimpleOperations. I am not completely happy with it, but it was the best I could think of. Completely open to suggestions.

And @fisker, I tried changing the lines of code from your comment back to using reduce, but then npm test failed. I think it's because xo needs to update to a newer release of eslint-plugin-unicorn once this rule is merged and updated in xo for it to work. Let me know if I am mistaken in that thinking.

[manovotny]

@sindresorhus @fisker do either of you have any feedback you'd like me to address?

[sindresorhus]

Can you fix the lint violation?

[manovotny]

Can you fix the lint violation?

Unless I am misunderstanding the failure, I believe it's coming from prevent-abbreviations.js. Perhaps an oversight of #1445?

I can address it here if you'd like or in a separate PR. Your call. Either way. It take a little more work (not a ton) because it's a dynamically generated message.

[manovotny]

@sindresorhus aside from the lint issue and my comment above regarding the issue, I believe everything else on the PR has been addressed.

[sindresorhus]

#1418 (comment)

[manovotny]

@sindresorhus all arr abbreviations removed in the rest file. Please let me know if / how you'd like me to resolve the lint issue.