Skip to content

Fixes clamp bug #36

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

Merged
merged 3 commits into from
Oct 24, 2018
Merged

Fixes clamp bug #36

merged 3 commits into from
Oct 24, 2018

Conversation

cqliu1
Copy link
Contributor

@cqliu1 cqliu1 commented Oct 22, 2018

Clamp uses falsy checks for min and max which causes problems when you pass in 0 for either parameter. This changes them to checks for null equality instead and adds a few unit tests.

@cqliu1 cqliu1 requested a review from rashidkpc October 22, 2018 19:21
@cqliu1 cqliu1 added the bug label Oct 22, 2018
@cqliu1 cqliu1 requested review from w33ble and removed request for rashidkpc October 23, 2018 19:45
@@ -21,14 +21,15 @@ const findClamp = (a, min, max) => {
*/

export function clamp(a, min, max) {
if (!min) return a;
if (min === null) return a;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be min == null? I'm not sure what you're testing for here, explicitly null, or just that something was passed in. A test case would help too.

Same with the max check below.

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 should be checking if any value was passed in for min, but I realize we want the same check for max.

@cqliu1
Copy link
Contributor Author

cqliu1 commented Oct 23, 2018

@w33ble I changed up the logic a bit in the clamp function. It now throws if either min or max is missing, so they are now both required arguments. If you leave either arg out, you could use the min or max instead or not use a function at all if you provide neither, so it's unnecessary for us to support expressions like clamp(3), clamp(2,1) and clamp(5,null,3)

if (max === null)
throw new Error("Missing maximum value. You may want to use the 'min' function instead");
if (min === null)
throw new Error("Missing minimum value. You may want to use the 'max' function instead");
Copy link
Contributor

Choose a reason for hiding this comment

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

🏆 for hints to the user about what they might want to use instead!

Copy link
Contributor

@w33ble w33ble left a comment

Choose a reason for hiding this comment

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

LGTM

@cqliu1 cqliu1 merged commit d9dae7b into elastic:master Oct 24, 2018
@cqliu1 cqliu1 deleted the fix/clamp-falsy-check branch October 25, 2018 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants