Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add no-reduce rule #704

Merged
merged 35 commits into from May 13, 2020
Merged

Add no-reduce rule #704

merged 35 commits into from May 13, 2020

Conversation

ankeetmaini
Copy link
Contributor

Fixes #623

@ankeetmaini
Copy link
Contributor Author

ankeetmaini commented May 3, 2020 •
edited

This is failing on existing reduce usages inside the project. Should I replace them? Also is there a way to know reduce is actually called on an Array.

e.g

const bar = {
  reduce() {
    console.log('foo');
  }
}

bar.reduce();

This will also fail. I don't think we can get type info in JS. Let me know!

@fisker
Copy link
Collaborator

fisker commented May 3, 2020

No, we can't know what foo is.

@ankeetmaini
Copy link
Contributor Author

No, we can't know what foo is.

Should I try and check the argument of CallExpression if it's a FunctionExpression|ArrowFunctionExpression? But it would then not trigger where the function is not defined inline. It'll just find an Identifier there.

fisker
fisker reviewed May 3, 2020
View reviewed changes
rules/no-reduce.js Outdated Show resolved Hide resolved
fisker
fisker requested changes May 3, 2020
View reviewed changes
rules/no-reduce.js Outdated Show resolved Hide resolved
rules/no-reduce.js Show resolved Hide resolved
rules/no-reduce.js Outdated Show resolved Hide resolved
fisker
fisker reviewed May 4, 2020
View reviewed changes
rules/no-reduce.js Outdated Show resolved Hide resolved
@fisker
Copy link
Collaborator

fisker commented May 4, 2020

@sindresorhus Forbid .reduceRight() too?

fisker
fisker reviewed May 4, 2020
View reviewed changes
test/no-reduce.js Outdated Show resolved Hide resolved
@ankeetmaini
Copy link
Contributor Author

ankeetmaini commented May 4, 2020 via email

fisker
fisker reviewed May 4, 2020
View reviewed changes
test/no-reduce.js Outdated Show resolved Hide resolved
fisker
fisker reviewed May 4, 2020
View reviewed changes
rules/no-reduce.js Outdated Show resolved Hide resolved
ankeetmaini
ankeetmaini commented May 4, 2020
View reviewed changes
rules/no-reduce.js Outdated Show resolved Hide resolved
fisker
fisker reviewed May 4, 2020
View reviewed changes
rules/no-reduce.js Outdated

const METHOD_SELECTOR = [
methodSelector({name: 'reduce', min: 1, max: 2}),
'[callee.object.property.name!="call"]'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it fails some tests if this is removed

a.b.call.reduce(() => {})

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But a.b.call should be a array, otherwise there is no reduce method.

@fisker
Copy link
Collaborator

fisker commented May 12, 2020

Let's add .reduceRight(), we are good to go.

fisker
fisker reviewed May 13, 2020
View reviewed changes
rules/utils/cartesian-product-samples.js Outdated
Comment on lines 17 to 26
let result = [];

for (const items of combinations) {
const {length} = items;
const index = indexRemaining % length;
indexRemaining = (indexRemaining - index) / length;
return [items[index], ...combination];
}, []);
result = [...result, items[index]];
}

return result;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is bad.

fisker
fisker reviewed May 13, 2020
View reviewed changes
rules/utils/cartesian-product-samples.js
@@ -14,12 +14,16 @@ module.exports = (combinations, length = Infinity) => {

const samples = Array.from({length: Math.min(total, length)}, (_, sampleIndex) => {
let indexRemaining = sampleIndex;
return combinations.reduceRight((combination, items) => {
const combination = [];
for (let i = combinations.length - 1; i >= 0; i--) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How to loop forward, if you interest https://github.com/fisker/fast-cartesian-product/blob/master/src/algorithms/calculate-from-index.js#L14

@fisker
Copy link
Collaborator

fisker commented May 13, 2020 •
edited

Personally, I use a reduce a lot, but most are summing things.

But since we can't tell what the function is doing, we have to report on all the reduce() calls.

I guess I won't use this rule.

Anyway, we are ready to merge.

fisker
fisker approved these changes May 13, 2020
View reviewed changes
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ankeetmaini Thank you for working on this!

@ankeetmaini
Copy link
Contributor Author

Thank you to you too for improving this PR! :)

@sindresorhus sindresorhus merged commit 5aed929 into sindresorhus:master May 13, 2020
sindresorhus added a commit that referenced this pull request May 13, 2020
@ankeetmaini @fisker @sindresorhus
Add no-reduce rule (#704)
06ed7ee 
Co-authored-by: fisker Cheung <lionkay@gmail.com>
Co-authored-by: Sindre Sorhus <sindresorhus@gmail.com>
@ankeetmaini ankeetmaini deleted the no-reduce branch May 13, 2020 06:48
@fisker fisker mentioned this pull request May 17, 2020
Closed
Repository owner locked as too heated and limited conversation to collaborators Aug 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fisker fisker fisker approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rule proposal: No reduce
3 participants
@ankeetmaini @fisker @sindresorhus