Skip to content

add Int.clamp and Float.clamp #90

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 5 commits into from
Jul 25, 2023
Merged

Conversation

glennsl
Copy link
Contributor

@glennsl glennsl commented Mar 5, 2023

I always find using min and max functions quite awkward, since piping them sort of inverts their meaning 3->max(2) == 3 and not piping them is sometimes awkward in itself if part of a larger pipeline, and especially if both are used together min(3, max(2, 1)) == ??. clamp fixes all of these problems.

@glennsl glennsl changed the title feat(int) add clamp add Int.clamp and Float.clamp Mar 5, 2023
@zth
Copy link
Collaborator

zth commented Mar 6, 2023

I like these and I reach for similar functionality here and there. So I'm in favor of including it. @cknitt, thoughts?

@cknitt
Copy link
Member

cknitt commented Mar 8, 2023

Personally I have not missed those yet, but I am not against adding them.

@jmagaram
Copy link
Contributor

jmagaram commented Mar 8, 2023

I think there is something a little random about adding this function and not dealing with max, min, and others. See the static methods on the .net Int. But there is max, min, minMagnitude, isOdd, isEven, etc.

https://learn.microsoft.com/en-us/dotnet/api/system.int32?view=net-7.0#methods

Rust...

https://doc.rust-lang.org/std/primitive.i32.html

And when should something go in the Math module? I just looked at the Math module by the way and it all works with floats. Annoying. Math.Int and Math.Float might be better.

@glennsl
Copy link
Contributor Author

glennsl commented Mar 9, 2023

I think there is something a little random about adding this function and not dealing with max, min, and others.

What's random about it? It fixes a specific problem with the existing functions while preserving the originals, since it's an objective of this project to stick close to JS APIs for familiarity.

@zth zth added this to the 0.3.0 milestone Mar 9, 2023
@jmagaram
Copy link
Contributor

jmagaram commented Mar 9, 2023

Is this in JavaScript already and JS developers expect it? I couldn't find it. I'm not understanding where this is coming from.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number

https://v2.ocaml.org/api/Int.html

@glennsl
Copy link
Contributor Author

glennsl commented Mar 9, 2023

It doesn't exist in JS, no, and the rationale is in the OP. I'm not sure what you're asking for beyond that.

@jmagaram
Copy link
Contributor

jmagaram commented Mar 9, 2023

I agree this is a useful thing and I'd be happy to see it in the library. I want the end result of this project to be a library that feels cohesive, well-thought-through, consistent. Good consistent naming, like getOr for default values or something similar for lazy is important. We should go to a consistent depth, especially within a single module. And so I'm asking what is so important about clamp that it belongs, but abs or isEven or max or add do not. I'm also wondering when something goes in Math and when it goes in Int. Both is ok, but it seems weird if Math.Int and Int diverge a lot. If we're going to add an Int.clamp shouldn't there be a similar Math.Int.clamp? By looking at the standard libraries for F#, Rust, and others we can get a good sense for what should be included in this project. Those projects had teams of people considering things and have been improved/augmented over time based on customer feedback.

@glennsl
Copy link
Contributor Author

glennsl commented Mar 9, 2023

I think this is the wrong place to have that discussion, and I also think that you've had it answered at least partly elsewhere. That a major focus of this library is familiarity with JS, not cohesiveness, and JS APIs unfortunately aren't very cohesive. It's still not a bad question, but I don't think it's a good idea to have the discussion scattered across multiple threads and to derail other issues and PRs with tangentially related questions.

@jmagaram
Copy link
Contributor

I'm not sure where to have a discussion. I posted a couple high-level questions on Issues - "should we fill out the option module?" and "what is the process for deciding what gets in?" - and got no response. There's no Discussions tab in this repo. You and everyone else on this project probably have a long history and know exactly what's going on. And I'm barging in asking annoying questions and providing opinions no one wants to hear. I like ReScript and want to help. I think I could contribute to the Option and Result modules if it is a goal to make those better. I will look at the "help wanted" issues and see if there is anything I can do there.

@glennsl
Copy link
Contributor Author

glennsl commented Mar 12, 2023

I wouldn't say you got no responses on the high-level issue you created for the Options module. I do get a sense that what you're asking for is mostly out-of-scope though, but it would be good to have more clarity on what is in and out of scope.

I'm not too sure either where to have these discussions, there is for example a rescript forum too, but it's certainly better to have high-level discussions in high-level issues than scattered across tangentially related PRs.

As for contributions, what's been actively asked for is mostly just documentation for what already exists. I think that would be a great place to start to get a sense of things as they are.

@zth
Copy link
Collaborator

zth commented Jul 24, 2023

@glennsl mind rebasing this one as well? Ready to merge after.

@glennsl
Copy link
Contributor Author

glennsl commented Jul 25, 2023

And done.

@zth zth merged commit 911bcf3 into rescript-lang:main Jul 25, 2023
@glennsl glennsl deleted the feat/int/clamp branch July 25, 2023 08:49
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