Skip to content

Consider the key's lowercase form while grouping#86

Merged
redox merged 1 commit intodevelopfrom
fix/85
Mar 24, 2016
Merged

Consider the key's lowercase form while grouping#86
redox merged 1 commit intodevelopfrom
fix/85

Conversation

@redox
Copy link
Contributor

@redox redox commented Mar 24, 2016

Fix #85

@algobot
Copy link

algobot commented Mar 24, 2016

By analyzing the blame information on this pull request, we identified @pixelastic to be a potential reviewer

@redox
Copy link
Contributor Author

redox commented Mar 24, 2016

@pixelastic What do you think about that?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.206% when pulling b45c603 on fix/85 into dd596e4 on develop.

src/lib/utils.js Outdated
throw new Error(`[groupBy]: Object has no key ${property}`);
}
let key = item[property];
let key = item[property] && item[property].toLowerCase();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the &&?

let key = item[property].toLowerCase(); should be enough

Copy link
Contributor

Choose a reason for hiding this comment

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

I think playground was failing otherwise

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because item[property] can be null

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we should update the if a few line above, where it is testing if it's undefined to also test null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I think it's fine to group hits sharing the null value together. It happens if - for a unknown reason - you don't have lvl1 for instance.

I can make the test more explicit if you want.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would make the code more explicit then.

let key = item[property];
if (key.toLowerCase) {
  let = key.toLowerCase();
}

Still not perfect (key can be an object with a toLowerCase field that is not a function...) but it expresses the intent of what you want to do more easily.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 99.206% when pulling adc0ab9 on fix/85 into dd596e4 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.219% when pulling e4705b3 on fix/85 into dd596e4 on develop.

@redox redox merged commit 89173e6 into develop Mar 24, 2016
@redox redox deleted the fix/85 branch March 24, 2016 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants