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

Is the bump method on the Version trait actually necessary? #49

Closed
mpizenberg opened this issue Oct 23, 2020 · 6 comments
Closed

Is the bump method on the Version trait actually necessary? #49

mpizenberg opened this issue Oct 23, 2020 · 6 comments

Comments

@mpizenberg
Copy link
Member

The ability to know the next version, expressed by the bump(&self) -> Self method on the Version trait does not seem to be vital. In the code, it is only used twice. The first time is for the definition of Range::exact(Version) -> Range. The second time is in the implementation of Display for an exact range, in order to have more readable reports. There, it is used to check if we have a range like (v1, Some(v2)) where v2 == v1.bump(). In such case it would be displayed v1 instead of v1 <= v < v2.

In addition, requiring a bump method seems like something that will prevent usage of things like pre-releases or versions based on dates, floating point numbers or other potentially valid versions otherwise.

If we were to remove the bump method, how could be represent ranges containing a single version that would still play nice with set operations like intersection and union?

@mpizenberg
Copy link
Member Author

floating point numbers

well maybe not that since it's not Ord

@Eh2406
Copy link
Member

Eh2406 commented Oct 26, 2020

Is it worth thinking even more meta, can we take a Range as a user provided type? What functionality do we need from a Range?

@mpizenberg
Copy link
Member Author

Is it worth thinking even more meta, can we take a Range as a user provided type? What functionality do we need from a Range?

Indeed. I'm not 100% sure, but I believe the negate, intersection and contains methods on a trait would be enough for everything (+ some trait bounds like Display and others).

@mpizenberg
Copy link
Member Author

Skimming through the code, Range::none() would probably also be needed, and should be renamed Range::empty() in my opinion. And Range::exact(v) would also be needed, and could also be renamed singleton.

The negate() method should also be renamed complement() I think to avoid confusion with term negation, and because Range really describes mathematical sets and so set terminology would be appropriate. I remember hesitating between Set and Range the first time when choosing the name but eventually settling on Range because Set would be confusing with those defined in std.

@Eh2406
Copy link
Member

Eh2406 commented Oct 27, 2020

seems like something that will prevent usage of things like pre-releases or versions based on dates, floating point numbers or other potentially valid versions otherwise.

For example:
a. Range([1.0.0, Some(1.1.0-beta)]) meaning something like normal versions between 1.0.0 and 1.1.0 and pre-releases of 1.1.0
b. Range([1.1.0-alpha, Some(1.2.0)])meaning something like normal versions between 1.1.0 and 1.2.0 and pre-releases of 1.1.0

So a.union(b) will give Range([1.0.0, Some(1.2.0)]) as 1.1.0-alpha < 1.1.0-beta so we can merge the intervals. Meaning something like normal versions between 1.0.0 and 1.2.0. But v=1.1.0-beta and v=1.1.0-alpha both are accepted by both a and b but not a.union(b), witch seams not great.

Given that kind of tricky corner case I am not seeing a way to support pre-releases without some way to redefine Range cc #39

@mpizenberg
Copy link
Member Author

This question is now obsolete since we merged in dev the bounded range design where each bound of the range can be inclusive or exclusive. We only need the order and can avoid the bump and lowest methods.

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

No branches or pull requests

2 participants