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

Method to bump semantic version #12499

Closed
gabriel-ss opened this issue Sep 19, 2022 · 13 comments · Fixed by #12834
Closed

Method to bump semantic version #12499

gabriel-ss opened this issue Sep 19, 2022 · 13 comments · Fixed by #12834

Comments

@gabriel-ss
Copy link
Contributor

Recently, I've been using Crystal at work to create some tools for the team and it's been awesome. One of the features of the std lib that has been very useful is the Semantic Version module; having a class with all the arithmetic for versions implemented helped during the development of many tools.

The only feature it currently lacks is a method to evaluate the next semantic version given the current version and the type of bump. Since this is a snippet that I reused in multiple projects, I thought that other people might find it useful enough to have in the std lib.

My implementation, created by extending the std class, can be used as:

current_version = SemanticVersion.new 1, 1, 1
next_minor = current_version.bump SemanticVersion::Bump::Minor
next_minor # => SemanticVersion(@build=nil, @major=1, @minor=2, @patch=0, @prerelease=SemanticVersion::Prerelease(@identifiers=[]))

Is there any interest in a PR adding something like this to the language?

@beta-ziliani
Copy link
Member

Nice! Getting ahead of a possible PR, I have a question. I understand that you have something like

enum Bump
  Major
  Minor
  Patch
end

How do you fancy the name Part instead?

@gabriel-ss
Copy link
Contributor Author

gabriel-ss commented Sep 20, 2022

Well, the enum is actually

  enum Bump
    Major
    Minor
    Patch
    PreMajor
    PreMinor
    PrePatch
    PreRelease
  end

So Part may be a little misleading about the nature of the operation, given that a single command could operate in a part and in the pre release at the same time. I could change this, though; there are other implementation details that are just conventions.

I used some CLI tools that do version operations (namely npm and poetry) as a base, but even then there are some differences in behavior, so I could make changes to make the implementation a better solution for the community needs.

@gabriel-ss
Copy link
Contributor Author

@beta-ziliani I made a draft with the snippets I've been using so far; any suggestions to make it more suitable for a PR?

@beta-ziliani
Copy link
Member

We'll look into it after 1.6 is released.

@beta-ziliani
Copy link
Member

Hi @gabriel-ss thanks for your patience! I checked your proposal. I'd say I prefer to have different methods for each part, as in bump_major, bump_minor, etc, and to split the pre-release. If you want to go from 1.1.2 to 1.2.0-alpha, it's perfectly reasonable to do it in two steps (first bump the minor, and then add the pre-release). WDYT?

@straight-shoota
Copy link
Member

As suggested in #7637 (comment), I think a method #copy_with would also make sense. It would work the same way as the implementation provided by the record macro. It allows to easily change individual aspects of a version and could be the basic building block on top of which the bump_* methods are implemented.

@gabriel-ss
Copy link
Contributor Author

Hey @beta-ziliani, using a #copy_with to build different methods could definitively be a way to go!

The only point is how exactly to handle the pre-releases; since a version with a pre-release is actually lower than the same version without one, doing it in two operations would mean to go backwards in the second one. Not really a problem, just a little quirk, and probably means that there is a part of the version handling logic that the applications will have to implement themselves, but then again maybe the need for this functionality doesn't come very often.

What do you have in mind for an API to manipulate the pre-releases?

@beta-ziliani
Copy link
Member

I'm not sure about bumping prereleases. It only works for scenarios like "rc", but not in those like "alpha", so in those cases I guess it's simpler to just go with #copy_with.

I have three possible implementations, probably the last one is my favorite, but not strongly.

  1. A method for each bump:
def bump_major(prerelease : String | Prerelease | Nil = nil, build : String? = nil)
def bump_minor(prerelease : String | Prerelease | Nil = nil, build : String? = nil)
def bump_patch(prerelease : String | Prerelease | Nil = nil, build : String? = nil)
def bump_prerelease(build : String? = nil) # precondition: it should have a prerelease! And I'm not sure it should exists
  1. One method + Enum:
# In this scenario Part makes more sense
Enum Part
  Major
  Minor
  Patch
  Prerelease # not sure about this one
end

def bump(part : Part, prerelease : String | Prerelease | Nil = nil, build : String? = nil)
  1. copy_with augmented with a unit value for triggering a bump in the specific field:
struct Bump
end

BUMP = Bump.new

def copy_with(major : Int | Bump = @major, minor : Int | Bump = @minor, patch : Int | Bump = @patch, prerelease : String | Prerelease | Nil = @prerealease, build : String? = @build)

@straight-shoota
Copy link
Member

straight-shoota commented Dec 2, 2022

My preference would be this:

  1. Generic #copy_with sans any special semantics and some bump methods for convenience.
def copy_with(major : Int32 = @major, minor : Int32 = @minor, patch : Int32 = @patch, prerelease : String | Prerelease | Nil = @prerealease, build : String? = @build)
def bump_major
def bump_minor
def bump_patch

Maybe also a #prerelease(String | Prerelease | Nil) convenience method.

@gabriel-ss
Copy link
Contributor Author

I agree with the arguments about the pre-releases; it's really hard to come up with a bump implementation that would work across multiple conventions. Maybe the best here is to just leave it open.

About the methods to bump versions, I find the semantic of

version.copy_with(minor: BUMP)

very elegant, but at the same time, if copy_with is supposed to be a low-level building block to, say, a package manager, I don't know if the special semantics would be appropriated. I'm divided between 3 and 4 in this case.

In both cases though, I think it would bring a great convenience by handling the edge cases, in special bumping the patch of a pre-release. I would be happy with either one.

@beta-ziliani
Copy link
Member

let's go with 4. sounds good?
it's true that a bump is not just a +1 in the part.

@gabriel-ss
Copy link
Contributor Author

Nice! Do you mind if I work on my previous implementation to make a new draft over the weekend? I'll let you guys know as soon as it's ready.

@gabriel-ss
Copy link
Contributor Author

gabriel-ss commented Dec 10, 2022

@straight-shoota, @beta-ziliani, I made a new draft! What are your thoughts on this one?

@straight-shoota
Copy link
Member

Looks good. Why don't you open a PR for it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants