Add increment methods #15

mh-cbon · 2016-06-11T09:31:20Z

No description provided.

mattfarina · 2016-06-20T18:46:41Z

This change turns the versions into mutable instances. Can you share some detail on your use case?

mh-cbon · 2016-06-20T19:57:04Z

yes indeed. I use it to bump my package automatically using one verb of prerelease patch minor major, see https://github.com/mh-cbon/gump

sdboyer · 2016-06-23T17:09:09Z

yeah, the change to mutability is a big loss. versions are value objects; if you need to change one, make a new one. plus, when #11 happens, this just won't work at all anymore.

if, instead, the methods returned a new *Version, that would be fine, IMO.

mattfarina · 2016-06-30T14:02:34Z

@sdboyer Can you explain why the change to immutability is a big deal? In a practical real world use sense. You say it's a "big loss" but that needs to be articulated.

sdboyer · 2016-06-30T16:00:46Z

Sure. There's a semantics reason, and a performance reason.

The performance reason is slightly indirect - basically, #11 needs to happen (I just added a more detailed explanation of the performance issues), and if Version methods no longer use pointer receivers and NewVersion() no longer returns a pointer, then you're operating on values, and mutating methods like these are a non-starter, as they simply won't work.

The semantics is a less clear-cut argument. Right now, even though pointer receivers are required for the methods, there are no methods that allow change. This makes them, in practice, immutable. This is lovely, because it means I can pass a version off to some function I don't directly control (e.g., an injected instance of an interface that takes a *Version argument), and be guaranteed that the value will be the same when the call completes. That guarantee is important, because it means that any sorted slice of Versions is guaranteed to still be sorted.

Adding these mutability methods takes away that guarantee. With that guarantee gone, I now have to code defensively: if I need to call any code that I'm not absolutely sure doesn't use these proposed methods, I first have to copy the version value into a new pointer address, then make the call. At least, I have to do that if I know that that version comes out of a sorted array...which, wait, now I have to keep track of that TOO? This becomes a big extra headache if you're trying to make a system in with real guarantees - that is, in which certain kinds of bad things literally can't happen.

To make matters worse, this defensive copying is explicitly about keeping the callee from being able to modify the real *Version value...which totally undermines the purpose of these methods in the first place! Any changes they may make by calling these methods would transparently disappear. How is the person writing the code I'm calling into supposed to know the difference between when it's OK to change a Version's value and when it's not?

Ordinarily, that's the sort of information I'd encode with function signatures: if I call a function with my Version, and that function's signature indicates it returns a Version, then that's probably an indication that both parties have agreed that the caller is potentially expecting a new or different version. Adding mutability like this forces it into EVERY call, taking away my ability to use the tools we have - the type system - to design an explicit, sane interface. Controlling when and where state change happens is...uh, important.

Perhaps most importantly, there's very little expressivity lost by making these methods return a new version, rather than modifying the existing one. IncMajor(), IncMinor(), and IncPatch() literally cannot return false, so there's no point in even checking it:

// As-is. No point in checking return because it tells us nothing, the method cannot fail
v.IncMajor()

// But if the signature is changed so a new version is returned instead:
func (v *Version) IncMajor() *Version { ... }
// Then the use of it becomes:
v = v.IncMajor()

I'm not sure what Inc() really adds here, TBH - IMO, it should should be removed, or at most made into a standalone func. The other two would probably need to be multi-return value, but that's no different than a call to NewVersion(), so no real DX loss.

Note that in my proposed form, it technically IS still mutable in a way that would propagate back to the "caller":

func SomeFuncWhereTheCallerDoesntWantAChange(v *Version) {
    // This would actually change the value at the pointer address,
    // making the caller rather unhappy
    *v = *v.IncMajor()

    // But this issue exists today, so it's no real change
    *v, _ = semver.NewVersion("1.0.0")
}

I'm banking on #11 going in to 2.x, at least, which will make that possibility go away.

mh-cbon · 2016-07-01T10:20:35Z

Hi,

thanks for all those valuable explanations @sdboyer

In a straight way, what would fit you more ?

I suggest to set a signature which is 2.x compatible by now.
Less efforts for the consumers when it will be to upgrade its dependencies.

func (v *Version) Inc(how string) (Version, error){}
func (v *Version) IncPatch() (Version, error){}
func (v *Version) IncMinor() (Version, error){}
func (v *Version) IncMajor() (Version, error){}
func (v *Version) SetPrerelease(metadata string) (Version, error){}
func (v *Version) SetMetadata(metadata string) (Version, error){}

type IncorrectIncrementVerb error
type IncorrectMetadataValue error
type IncorrectPrereleaseValue error

Inc is totally arguable in many ways. I just don t see the interest to let others write into a lost standalone function what makes sense in a very particular place.

IMHO, i m more concerned by SetPrerelease which really is of very little help for the end consumer.
I had prefer to have an IncPrerelease, but to do that, some assumptions must be taken which does not comply with the "standard" of semver.
Leads me to do that sorts of things.

sdboyer · 2016-07-01T13:34:10Z

In a straight way, what would fit you more ?

Sorry, yeah, I should have actually made the proposal concrete. Here's what I would propose - pretty close to what you have:

func (v *Version) IncPatch() *Version {}
func (v *Version) IncMinor() *Version {}
func (v *Version) IncMajor() *Version {}
func (v *Version) SetPrerelease(metadata string) (*Version, error) {}
func (v *Version) SetMetadata(metadata string) (*Version, error) {}

type IncorrectMetadataValue error
type IncorrectPrereleaseValue error

This drops the error return from IncPatch(), IncMinor() and IncMajor() because those really can't fail, modulo integer overflow (which...is that really the lib's job to defend against? idk).

As you said, Inc() is arguable; my preference is to omit it, so I've omitted it there, but I don't really care that much either way. If it is there, though, then IncorrectIncrementVerb has to come back.

IMHO, i m more concerned by SetPrerelease which really is of very little help for the end consumer.
I had prefer to have an IncPrerelease, but to do that, some assumptions must be taken which does not comply with the "standard" of semver.

Agreed, this is the tricky one. I think it's the kind of thing we could also easily implement later, without much harm. But if we have a notion of incrementing prereleases, then we have to internally define the ordering relationship within prereleases, and have that extend to all the other order-sensitive code in the lib.

I suggest to set a signature which is 2.x compatible by now.

We've let the 2.x branch sit for a while, but in my mind at least, the big thing blocking a 2.x release is #11. I think it's fine to have this change go in, also pull it up to 2.x, and then when #11 happens we just change all the pointer receivers and returns to values.

mattfarina · 2016-07-01T14:05:17Z

We should drop Inc because of string parsing and deciding on the rules for that. If there's a major call for it later we can talk about adding it.

There's a question of decrement. Why should there be a case for incrementing and not decrementing?

sdboyer · 2016-07-01T14:18:38Z

Hah. Good point. So:

func (v *Version) IncPatch() *Version {}
func (v *Version) DecPatch() *Version {}
func (v *Version) IncMinor() *Version {}
func (v *Version) DecMinor() *Version {}
func (v *Version) IncMajor() *Version {}
func (v *Version) DecMajor() *Version {}
func (v *Version) SetPrerelease(metadata string) (*Version, error) {}
func (v *Version) SetMetadata(metadata string) (*Version, error) {}

type IncorrectMetadataValue error
type IncorrectPrereleaseValue error

?

mh-cbon · 2016-07-01T15:48:10Z

mhhhhh. Decrement a version.

What should happen if you are 1.0.0 and would like to go down ? 0.0.9999999999999999999999999999999999999 ?
or just 0.0.9 ?

I guess you never really decrement a version, rather than restore an existing one, which means you would do semver.NewVersion("old ver str") according to some external version history reference.

I m not sure why It returns a Version pointer ?
Won t it be confusing for the consumer when it will read the method signature ?
The pointer can mean this, as if it was chainable on the current instance, or a pointer to a new value like we d like, right ?
In fact the consumer won t know unless he reads the code or some comments.

I d rather return value as the instance must remain immutable.

func (v *Version) IncPatch() Version {}
func (v *Version) IncMinor() Version {}
func (v *Version) IncMajor() Version {}
func (v *Version) SetPrerelease(metadata string) (Version, error) {}
func (v *Version) SetMetadata(metadata string) (Version, error) {}

type IncorrectMetadataValue error
type IncorrectPrereleaseValue error

sdboyer · 2016-07-02T01:45:53Z

hmm hmm hmm

What should happen if you are 1.0.0 and would like to go down ? 0.0.9999999999999999999999999999999999999 ?
or just 0.0.9 ?

maybe this is mitigated by having three separate methods: if you have 1.0.0, and you call DecPatch, you get 1.0.0. DecMinor also returns 1.0.0. DecMajor returns 0.0.0. Because otherwise...yeah, there's no good solution.

Having these semantics would also sorta strengthen the case for #19, I think.

I m not sure why It returns a Version pointer ?
Won t it be confusing for the consumer when it will read the method signature ?
The pointer can mean this, as if it was chainable on the current instance, or a pointer to a new value like we d like, right ?

I suspect it would be, confusing, indeed :) Which is why I was only really envisioning this as a temporary measure - that #11 has to happen before this API would really make sense. The problem with not returning a *Version is that all the other methods take a pointer receiver, so you're returning a not-useful thing for most normal purposes.

...though, come to think of it, I guess there's no real reason we have to use pointer receivers for new methods. We could just do this:

func (v Version) IncPatch() Version {}
func (v Version) IncMinor() Version {}
func (v Version) IncMajor() Version {}
func (v Version) SetPrerelease(metadata string) (Version, error) {}
func (v Version) SetMetadata(metadata string) (Version, error) {}

type IncorrectMetadataValue error
type IncorrectPrereleaseValue error

And then the caller can take the address of the returned value, if they need the other methods.

mh-cbon · 2016-07-02T11:28:31Z

The problem with not returning a *Version is that all the other methods take a pointer receiver, so you're returning a not-useful thing for most normal purposes.

Oh yeah i guess you ve right, being still learning go, that sort of things are not yet straight to me.

Yet i suspect that using value as context in func (v Version) ... {} will lead to some mumble jumble.
To increment a version, if i don t mistake you d have to do *ver.Inc(), then &ver.Patch() to get its patch value. :x :x :x

Ahm, when is 2.x released ? =)

sdboyer · 2016-07-02T12:00:26Z

being still learning go, that sort of things are not yet straight to me.

ah, well, welcome! 😄 totally reasonable thing to not have quite straight yet.

if i don t mistake you d have to do *ver.Inc(), then &ver.Patch() to get its patch value. :x :x :x

not quite - that's part of why I suggested it. see the spec re: method sets: if you have a variable of type T, its method set includes only those methods that receive T. If you have the pointer type *T, then its method set includes both the methods that receive T and *T. So using it would look like this:

// v is a *Version
v := semver.NewVersion("1.0.0")

// This assigns the value returned from IncMinor() to the address from our original 
// pointer, v, so v is now 1.1.0
*v = v.IncMinor()

v2 := v.IncMajor()
// v2 is a non-pointer Version, containing 2.1.0. Original value at pointer address
// is unchanged. However, none of the methods requiring a pointer are accessible,
// though our new ones are:
v2.Major() // method not accessible
v2 = v2.IncPatch() // works, v2 is now 2.1.1

v3 := &v.IncMajor()
// v3 is a new pointer to a new address, separate from v's, and has access to all
// the normal methods. So, this works:
v3.Major()

So, it's still a little awkward, but at least we have our immutability guarantees encoded in the type signature.

I think it'd maybe be fine to leave this for 2.x, though, with the expectation that it's only tagged for release once #11 goes in. At that point, everything's consistently using a value receiver, so there's no headache at all.

Gives me some motivation to come back and wrap up things here, so we can get that tagged release out :)

mh-cbon · 2016-07-05T09:31:58Z

There it is!

sdboyer · 2016-07-05T11:18:02Z

version.go

+  vNext.pre = ""
+	vNext.patch = 0
+	vNext.minor = v.minor+1
+	vNext.original = vNext.String()


Probably not safe to to try to preserve the original string, as it now describes the old value, not the current one

Sorry, I don t get your point.

After a call to Inc*, NewVersion.original is set to NewVersion.String.
It will reflect the most recent changes as if NewVersion was called.

In other words

v, _ := NewVersion("1.2.3") vv := v.IncPatch() fmt.Println(vv.Original()) // => 1.2.4 fmt.Println(v.Original()) // => 1.2.3

BTW,
About original, I don t get the point of keeping it. Neither its intent.
It is the same thing in constraints.go with the orig field.
They exists, but they are unused within the lib.

sdboyer · 2016-07-06T00:53:37Z

(moving discussion out of outdated line comments)

Sorry, I don t get your point.

After a call to Inc*, NewVersion.original is set to NewVersion.String.
It will reflect the most recent changes as if NewVersion was called.

Sorry, you're totally right - I misread the code. You do handle it correctly. However...(see below)

BTW,
About original, I don t get the point of keeping it. Neither its intent.

The purpose of it is being able to precisely recreate the user's input, given that there are multiple ways a given version or constraint could be expressed in the input string. That is, "v1.0.0" and "1.0.0" both result in the same version. This matters for tools like glide, which often have to write out a file with versions in it, and it's quite valuable for there to be as little change as possible in the file. By retaining the user's original input, that goal is easily achieved.

On further reflecting on these reasons, I think that it'd actually be better to either not populate the original field on versions at all, or else sniff the original to see if the leading v is present. If it is, then make sure it's present on the newly-returned version. If not, make sure it isn't.

Seem sane?

mh-cbon · 2016-07-06T11:32:23Z

Works for me. Will do another pass later to sniff it and and keep the original v prefix accordingly.

mh-cbon · 2016-07-06T14:08:41Z

one stupid q i have here, the SemverRegexp won t accept V1.2.3, it s case sensitive.
As this, obviously, wrong naming style is supported, i believe the lib should do its best.

I d suggest to change this if you don t foresee any problems.

sdboyer · 2016-07-06T17:19:08Z

V1.2.3 shouldn't be accepted - only v1.2.3. While none of this v-prefixing is actually in the semver spec, only the lowercase v is what anyone accepts in the wild, AFAIK. I think it's good as-is.

mh-cbon · 2016-07-06T18:03:47Z

I keep trying ; )

Strongly disagree. The user that will face that error message Invalid Semantic Version for a case sensitivity issue will feel blocked.
As to him, and i kind of agree to that as the lib already supports v, it appears to be a correct semver value.

It s really not obvious for that kind of end user, at best he ll think the lib he is consuming is dumb / buggy, leaving it with a bad impression of it, which the lib does not deserve at all.

sdboyer · 2016-07-06T19:29:08Z

Either way, it's orthogonal to the issue we're dealing with here. Probably best to open a new issue.

mh-cbon · 2016-07-06T21:13:50Z

good to go, i hope

mh-cbon · 2016-07-24T11:09:08Z

hi @sdboyer @mattfarina this change fixes increments of prereleased version number.
IE: a version like 1.2.3-beta+meta should change to 1.2.3 not 1.2.4

I also reviewed some comments here and there.

mattfarina · 2016-10-28T20:50:10Z

version.go

@@ -129,6 +137,95 @@ func (v *Version) Metadata() string {
 	return v.metadata
 }

+// OrignalVPrefix returns the original 'v' prefix if any.
+func (v *Version) OrignalVPrefix() string {


Why is OrignalVPrefix an exported function? It only appears to be needed internally.

mattfarina · 2016-10-28T20:50:59Z

Note, I'd like to merge this soon and release.

mh-cbon · 2016-10-29T12:30:41Z

privatized OrignalVPrefix to originalVPrefix (note the typo)
clarified comments for incpatch method
squashed the commits

Add increment methods

mattfarina · 2016-11-04T13:59:25Z

@mh-cbon Thanks for the contribution. A release will be coming along shortly.

In Masterminds#11 and Masterminds#15 there were some good arguments made in favor of changing the API to use value receivers and function arguments. While Masterminds/semver apparently didn't adopt this breaking API change, this is an attempt to try it out in a hard fork.

sdboyer reviewed Jul 5, 2016
View reviewed changes

mattfarina reviewed Oct 28, 2016

View reviewed changes

mh-cbon force-pushed the master branch 2 times, most recently from e0e1160 to d4c6b7b Compare October 29, 2016 12:13

Add methods to increment version number

de6c977

mh-cbon force-pushed the master branch from d4c6b7b to de6c977 Compare October 29, 2016 12:23

mattfarina merged commit de6c977 into Masterminds:master Nov 4, 2016

mattfarina added a commit that referenced this pull request Nov 4, 2016

Merge pull request #15 from mh-cbon/master

bc8fb81

Add increment methods

wfscheper mentioned this pull request Sep 14, 2023

refactor!: consistently use values instead of pointers wfscheper/semver#1

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add increment methods #15

Add increment methods #15

mh-cbon commented Jun 11, 2016

mattfarina commented Jun 20, 2016

mh-cbon commented Jun 20, 2016

sdboyer commented Jun 23, 2016 •

edited

Loading

mattfarina commented Jun 30, 2016

sdboyer commented Jun 30, 2016 •

edited

Loading

mh-cbon commented Jul 1, 2016

sdboyer commented Jul 1, 2016

mattfarina commented Jul 1, 2016

sdboyer commented Jul 1, 2016

mh-cbon commented Jul 1, 2016 •

edited

Loading

sdboyer commented Jul 2, 2016

mh-cbon commented Jul 2, 2016

sdboyer commented Jul 2, 2016 •

edited

Loading

mh-cbon commented Jul 5, 2016

sdboyer Jul 5, 2016

mh-cbon Jul 5, 2016

sdboyer commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

sdboyer commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

sdboyer commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

mh-cbon commented Jul 24, 2016

mattfarina Oct 28, 2016

mattfarina commented Oct 28, 2016

mh-cbon commented Oct 29, 2016

mattfarina commented Nov 4, 2016

Add increment methods #15

Add increment methods #15

Conversation

mh-cbon commented Jun 11, 2016

mattfarina commented Jun 20, 2016

mh-cbon commented Jun 20, 2016

sdboyer commented Jun 23, 2016 • edited Loading

mattfarina commented Jun 30, 2016

sdboyer commented Jun 30, 2016 • edited Loading

mh-cbon commented Jul 1, 2016

sdboyer commented Jul 1, 2016

mattfarina commented Jul 1, 2016

sdboyer commented Jul 1, 2016

mh-cbon commented Jul 1, 2016 • edited Loading

sdboyer commented Jul 2, 2016

mh-cbon commented Jul 2, 2016

sdboyer commented Jul 2, 2016 • edited Loading

mh-cbon commented Jul 5, 2016

sdboyer Jul 5, 2016

Choose a reason for hiding this comment

mh-cbon Jul 5, 2016

Choose a reason for hiding this comment

sdboyer commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

sdboyer commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

sdboyer commented Jul 6, 2016

mh-cbon commented Jul 6, 2016

mh-cbon commented Jul 24, 2016

mattfarina Oct 28, 2016

Choose a reason for hiding this comment

mattfarina commented Oct 28, 2016

mh-cbon commented Oct 29, 2016

mattfarina commented Nov 4, 2016

sdboyer commented Jun 23, 2016 •

edited

Loading

sdboyer commented Jun 30, 2016 •

edited

Loading

mh-cbon commented Jul 1, 2016 •

edited

Loading

sdboyer commented Jul 2, 2016 •

edited

Loading