version: add new package.

version: add new package.



https://code.launchpad.net/~rogpeppe/juju/go-add-versions-package/+merge/102905

(do not edit description out of merge proposal)

[rog, 2012-04-20 17:13:18]

Please take a look.

[TheMue, 2012-04-20 17:19:47]

On 2012/04/20 17:13:18, rog wrote:
> Please take a look.

LGTM, only usage of go check to be consistent.

[fwereade, 2012-04-20 17:24:43]

LGTM apart from uncertainty about minor version compatibility, I'd appreciate a
more detailed explanation.

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go
File environs/version/version.go (right):

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go#...
environs/version/version.go:12: // Version represents a parsed version. See
http://semvar.org/ for
s/semvar/semver/

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go#...
environs/version/version.go:93: // lessIds returns whether the slice of
identifiers a is equal to b,
s/lessIds/eqIds/

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go#...
environs/version/version.go:180: return false
I'm not 100% certain about this. Explain?

[niemeyer, 2012-04-23 13:14:32]

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go
File environs/version/version.go (right):

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go#...
environs/version/version.go:19: Build      []string // The build version
(dot-separated elements)
These look more complex than necessary. Something like this would do it:

 Pre int
 Dev bool

The pre releases and build releases, if we want to use them, should look simple
and consistent. For example:

    2.0.3-pre1
    2.0.3-pre2
    2.0.4+dev

When parsing, we can scan for the logic we expect, precisely:

    $MAJOR.$MINOR.$PATCH(-pre$PRE|+dev)

If it's not in that form, we return an error. This drops most of the logic
below.

https://codereview.appspot.com/6082044/diff/2001/environs/version/version.go#...
environs/version/version.go:154: func (v *Version) Less(w *Version) bool {
We can have a single method:

type Flags uint8

const (
    Major Flags = 1 << iota
    Dev
)

func (v *Version) Upgrades(w *Version, flags Flags) bool

This returns true if v is a valid upgrade for w, using the rules we defined.
That's all we really care about, I believe, and it'll be nice to have that
knowledge clearly written down in a single place.

[niemeyer, 2012-04-24 00:15:47]

This should be adapted to the conclusion of the thread in the ML.

[rog, 2012-04-25 15:55:48]

Please take a look.

[niemeyer, 2012-04-25 16:13:58]

Good stuff. LGTM, with some straightforward suggestions.

https://codereview.appspot.com/6082044/diff/8001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode13
version/version.go:13: // Version represents a parsed version.
// Version represents a juju version.
// As a convention, when bugs are fixed the patch number
// is incremented, when new features are added the minor
// number is incremented and patch is reset, and when
// compatibility is broken the major version is incremented
// and minor and patch are reset.
// If any of the numbers is odd, though, it indicates
// that the release is still in development.

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode17
version/version.go:17: Patch int // The patch version number.
The inline comments are obvious given context.

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode57
version/version.go:57: func (v *Version) String() string {
s/*//?

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode83
version/version.go:83: return isOdd(v.Major) ||
That will probably look nicer as a one-liner:

return isOdd(v.Major) || isOdd(v.Minor) || isOdd(v.Patch)

[rog, 2012-04-25 16:23:00]

*** Submitted:

version: add new package.

R=
CC=
https://codereview.appspot.com/6082044

https://codereview.appspot.com/6082044/diff/8001/version/version.go
File version/version.go (right):

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode13
version/version.go:13: // Version represents a parsed version.
On 2012/04/25 16:13:59, niemeyer wrote:
> // Version represents a juju version.
> // As a convention, when bugs are fixed the patch number
> // is incremented, when new features are added the minor
> // number is incremented and patch is reset, and when
> // compatibility is broken the major version is incremented
> // and minor and patch are reset.
> // If any of the numbers is odd, though, it indicates
> // that the release is still in development.

Done with minor edits.

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode17
version/version.go:17: Patch int // The patch version number.
On 2012/04/25 16:13:59, niemeyer wrote:
> The inline comments are obvious given context.

Done.

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode57
version/version.go:57: func (v *Version) String() string {
On 2012/04/25 16:13:59, niemeyer wrote:
> s/*//?

Done.

https://codereview.appspot.com/6082044/diff/8001/version/version.go#newcode83
version/version.go:83: return isOdd(v.Major) ||
On 2012/04/25 16:13:59, niemeyer wrote:
> That will probably look nicer as a one-liner:
> 
> return isOdd(v.Major) || isOdd(v.Minor) || isOdd(v.Patch)

Done.