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

Clamp #1564

Merged
merged 1 commit into from
Jan 8, 2016
Merged

Clamp #1564

merged 1 commit into from
Jan 8, 2016

Conversation

ccorcos
Copy link
Contributor

@ccorcos ccorcos commented Dec 26, 2015

#1563

R.clamp(1, 10, -1) // => 1
R.clamp(1, 10, 11) // => 10
R.clamp(1, 10, 4)  // => 4

@ccorcos ccorcos changed the title create clamp function Clamp Dec 26, 2015
@davidchambers
Copy link
Member

Should we throw if min > max?

@ccorcos
Copy link
Contributor Author

ccorcos commented Dec 27, 2015

good idea

@ccorcos
Copy link
Contributor Author

ccorcos commented Dec 27, 2015

done

* R.clamp(1, 10, 4) // => 4
*/
module.exports = _curry3(function clamp(min, max, value) {
if (min > max) throw new Error('min must be greater than max in clamp(min, max, value)')
Copy link
Member

Choose a reason for hiding this comment

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

This should read min must not be greater than max. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ccorcos
Copy link
Contributor Author

ccorcos commented Dec 30, 2015

hmm. not sure whats going on here with that error...

* R.clamp(1, 10, 4) // => 4
*/
module.exports = _curry3(function clamp(min, max, value) {
if (min > max) throw new Error('min must not be greater than max in clamp(min, max, value)')
Copy link
Member

Choose a reason for hiding this comment

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

In order to appease the linter, you'll need to write this like so:

if (...) {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. done

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 2, 2016

ok @davidchambers, added some tests for letters as well. I'm not sure what you want to do about ./shared/eq though since it doesnt exist. I could import ../src/equals instead.

@davidchambers
Copy link
Member

@ccorcos, see test/shared/eq.js. ;)

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 5, 2016

Hmm. I'm confused. Did I fork off of 0.14?! https://github.com/ccorcos/ramda/blob/master/package.json#L27

I'll have to rebase when I get home.

@davidchambers
Copy link
Member

You'll need to update your fork's master branch:

$ git checkout master
$ git pull upstream master
$ git push origin master

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 5, 2016

awesome. done.

var _curry3 = require('./internal/_curry3');

/**
* Clamps a number within a range.
Copy link
Member

Choose a reason for hiding this comment

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

Shall we mention that this function applies to any type which supports ordering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure: how about this?

Clamps any type that supports ordering within a range.

Copy link
Member

Choose a reason for hiding this comment

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

In light of #1578 we should probably devote one sentence to the common case and one sentence to the abstraction. What do you think, @CrossEye?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree.

Restricts a number to be within a range.

Also works for other ordered types such as Strings and Dates.  (see Type::Ord)

That last bit it just a bit of fantasy, but something I'd love to be able to support.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 7, 2016

Alright, so what do you guys think? My first Ramda contribution? :)

@davidchambers
Copy link
Member

Looks good, aside from the two outstanding issues:

  • (see Type::Ord) should be removed; and
  • the empty line at the top of test/clamp.js should be removed.

Once you've addressed these minor issues I'll happily merge this pull request. :)

@CrossEye
Copy link
Member

CrossEye commented Jan 8, 2016

🌿, once those minor changes are made.

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 8, 2016

Boom 🌿

CrossEye added a commit that referenced this pull request Jan 8, 2016
@CrossEye CrossEye merged commit 753e09b into ramda:master Jan 8, 2016
@CrossEye
Copy link
Member

CrossEye commented Jan 8, 2016

Thank you very much!

@davidchambers
Copy link
Member

🌳 Thanks for persevering. :)

@ccorcos
Copy link
Contributor Author

ccorcos commented Jan 8, 2016

Of course 👍 I look forward to contributing more now that I got the first one out of the way.

@buzzdecafe buzzdecafe mentioned this pull request Mar 26, 2016
@xgrommx
Copy link

xgrommx commented Mar 26, 2016

About clamp maybe it will be more general?

const clamp = curry((min, max, value) => Math.max(min, Math.min(max, value)));

Also if you add clamp why not lerp?

const lerp = curry((start, end, alpha) => start*(1-alpha)+end*alpha

It is linear interpolation https://en.wikipedia.org/wiki/Linear_interpolation http://codepen.io/ma77os/pen/KGIEh

@CrossEye
Copy link
Member

@xgrommx:

About clamp maybe it will be more general?

Perhaps I'm misunderstanding. The current implementation handles that case, but should work with any ordered type.

Also if you add clamp why not lerp?

While that is certainly a useful function, my first thought is that it belongs in The Cookbook instead. I certainly would not object to hearing arguments to the contrary though if you disagree.

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.

4 participants