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

Add LowerBounded/UpperBounded traits #210

Merged
merged 1 commit into from
Apr 10, 2021
Merged

Conversation

clarfonthey
Copy link
Contributor

Potential solution for #208. With a breaking change, these could simply become supertraits of Bounded, but until then, we have to deal with blanket implementations.

I added both simply because it was easy to do so, although we could opt to not require both. I don't see it being that negative to include both, however.

@regexident
Copy link
Contributor

regexident commented Mar 25, 2021

Both trait LowerBounded and trait UpperBounded on their own look like a reasonable addition to num-traits. In light of the existing trait Bounded however they —and especially their blanket implementations— feel wrong to me as trait Bounded —like you said— should clearly be redefined in terms of trait Bounded: LowerBounded + UpperBounded {}, not the other way round.

The problem these traits aim to address is one of many arguments in favor of going thru with a future breaking release, while the proposed compromise/workaround might end up working against just that.

@cuviper either way this issue deserves an entry in the tracking issue, I think.

@cuviper
Copy link
Member

cuviper commented Apr 10, 2021

I added that to the tracking issue. Otherwise, this looks fine to me, thanks!

bors r+

@bors
Copy link
Contributor

bors bot commented Apr 10, 2021

@bors bors bot merged commit 305532d into rust-num:master Apr 10, 2021
@clarfonthey clarfonthey deleted the bounded branch April 10, 2021 01:38
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.

3 participants