names: add ParseTag

names: add ParseTag

This simplifies State.entity (shortly to be State.Entity)
and the names package itself.

https://code.launchpad.net/~rogpeppe/juju-core/362-names-parse-tag/+merge/178619

(do not edit description out of merge proposal)

[rog, 2013-08-06 09:21:08]

Please take a look.

[dimitern, 2013-08-06 09:35:39]

Looks really good, just a few suggestions.

https://codereview.appspot.com/12473043/diff/6001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode1
names/machine.go:1: // Copyright 2013 Canonical Ltd.
I missed that? Darn..

https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode31
names/machine.go:31: func machineFromTagName(name string) string {
machineFromTag(tag string) string { ... please.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (left):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#oldcode12
names/tag_test.go:12: type tagKindSuite struct{}
Please leave the old name here.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#newcode119
names/tag_test.go:119: // TODO environment and user, when they have Tag
functions.
// TODO(rog) ...

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#newcode124
names/tag_test.go:124: c.Logf("test %d. %q expectKind %q", i, test.tag,
test.expectKind)
test %d: ...

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go
File names/tagkind.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go#newcode28
names/tagkind.go:28: UnitTagKind:    unitFromTagName,
unitFromTag and machineFromTag please. There's no concept of a "tag name".

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go#newcode77
names/tagkind.go:77: if fromTag := fromTagName[kind]; fromTag != nil {
if fromTagFunc := fromTag[kind]; fromTagFunc != nil {

https://codereview.appspot.com/12473043/diff/6001/names/unit.go
File names/unit.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/unit.go#newcode31
names/unit.go:31: func unitFromTagName(name string) string {
s/unitFromTagName/unitFromTag/

https://codereview.appspot.com/12473043/diff/6001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/12473043/diff/6001/state/state_test.go#newcode...
state/state_test.go:1391: c.Logf("test %d. entity %q", i, name)
test %d: ...

[rog, 2013-08-06 09:50:08]

https://codereview.appspot.com/12473043/diff/6001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode31
names/machine.go:31: func machineFromTagName(name string) string {
On 2013/08/06 09:35:40, dimitern wrote:
> machineFromTag(tag string) string { ... please.

it's not from a tag - it's from the name part of the tag, hence the different
name.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (left):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#oldcode12
names/tag_test.go:12: type tagKindSuite struct{}
On 2013/08/06 09:35:40, dimitern wrote:
> Please leave the old name here.

it tests more than just tag kinds (it's also testing ParseTag) so i think the
new name is appropriate.
note also that the file has been renamed.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#newcode124
names/tag_test.go:124: c.Logf("test %d. %q expectKind %q", i, test.tag,
test.expectKind)
On 2013/08/06 09:35:40, dimitern wrote:
> test %d: ...

% pwd
/home/rog/src/go/src/launchpad.net/juju-core
% g -r '"test %d\.'  | wc
     23     147    1776
% g -r '"test %d\:'  | wc
     34     222    2591
%

There's a fair weight to either side. As (I believe) the instigator of this
particular idiom, I prefer the "." - it's not a sentence that benefits from a
colon, and it looks nice as a title.

If you want to go through the whole source tree standardising this, I won't
object, but I'll leave this as is for the time being.

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go
File names/tagkind.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go#newcode28
names/tagkind.go:28: UnitTagKind:    unitFromTagName,
On 2013/08/06 09:35:40, dimitern wrote:
> unitFromTag and machineFromTag please. There's no concept of a "tag name".

a tag has two parts - these functions operate on the second part,
not the whole thing as would be implied if this was called
unitFromTag.

as an arbitrary decision, i'm calling that the tag "name"
(as opposed to the id, as passed to UnitTag, MachineTag etc).

better suggestions for a name other than "name" are welcome,
but fromTag is wrong.

[dimitern, 2013-08-06 10:00:28]

LGTM, but I'd like to see the "test %d." changed to "test %d:" please.

https://codereview.appspot.com/12473043/diff/6001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode31
names/machine.go:31: func machineFromTagName(name string) string {
On 2013/08/06 09:50:08, rog wrote:
> On 2013/08/06 09:35:40, dimitern wrote:
> > machineFromTag(tag string) string { ... please.
> 
> it's not from a tag - it's from the name part of the tag, hence the different
> name.

Ok, as agreed online - fromTagSuffix(suffix string) ...

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (left):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#oldcode12
names/tag_test.go:12: type tagKindSuite struct{}
On 2013/08/06 09:50:08, rog wrote:
> On 2013/08/06 09:35:40, dimitern wrote:
> > Please leave the old name here.
> 
> it tests more than just tag kinds (it's also testing ParseTag) so i think the
> new name is appropriate.
> note also that the file has been renamed.

Ok, I can live with that.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#newcode124
names/tag_test.go:124: c.Logf("test %d. %q expectKind %q", i, test.tag,
test.expectKind)
On 2013/08/06 09:50:08, rog wrote:
> On 2013/08/06 09:35:40, dimitern wrote:
> > test %d: ...
> 
> % pwd
> /home/rog/src/go/src/launchpad.net/juju-core
> % g -r '"test %d\.'  | wc
>      23     147    1776
> % g -r '"test %d\:'  | wc
>      34     222    2591
> %
> 
> There's a fair weight to either side. As (I believe) the instigator of this
> particular idiom, I prefer the "." - it's not a sentence that benefits from a
> colon, and it looks nice as a title.
> 
> If you want to go through the whole source tree standardising this, I won't
> object, but I'll leave this as is for the time being.

Please, make it "test %d:", as you can see it's more common in the codebase and
I personally prefer that, as it makes more sense - it's not a sentence by
itself, more like a prefix, hence the :. I'm not saying change every other
occurrence, just this one.

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go
File names/tagkind.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go#newcode28
names/tagkind.go:28: UnitTagKind:    unitFromTagName,
On 2013/08/06 09:50:08, rog wrote:
> On 2013/08/06 09:35:40, dimitern wrote:
> > unitFromTag and machineFromTag please. There's no concept of a "tag name".
> 
> a tag has two parts - these functions operate on the second part,
> not the whole thing as would be implied if this was called
> unitFromTag.
> 
> as an arbitrary decision, i'm calling that the tag "name"
> (as opposed to the id, as passed to UnitTag, MachineTag etc).
> 
> better suggestions for a name other than "name" are welcome,
> but fromTag is wrong.

fromTagSuffix, as agreed.

[rog, 2013-08-06 10:04:20]

Please take a look.

https://codereview.appspot.com/12473043/diff/6001/names/machine.go
File names/machine.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/machine.go#newcode31
names/machine.go:31: func machineFromTagName(name string) string {
On 2013/08/06 10:00:28, dimitern wrote:
> On 2013/08/06 09:50:08, rog wrote:
> > On 2013/08/06 09:35:40, dimitern wrote:
> > > machineFromTag(tag string) string { ... please.
> > 
> > it's not from a tag - it's from the name part of the tag, hence the
different
> > name.
> 
> Ok, as agreed online - fromTagSuffix(suffix string) ...

Done.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go
File names/tag_test.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#newcode119
names/tag_test.go:119: // TODO environment and user, when they have Tag
functions.
On 2013/08/06 09:35:40, dimitern wrote:
> // TODO(rog) ...

Done.

https://codereview.appspot.com/12473043/diff/6001/names/tag_test.go#newcode124
names/tag_test.go:124: c.Logf("test %d. %q expectKind %q", i, test.tag,
test.expectKind)
On 2013/08/06 10:00:28, dimitern wrote:
> On 2013/08/06 09:50:08, rog wrote:
> > On 2013/08/06 09:35:40, dimitern wrote:
> > > test %d: ...
> > 
> > % pwd
> > /home/rog/src/go/src/launchpad.net/juju-core
> > % g -r '"test %d\.'  | wc
> >      23     147    1776
> > % g -r '"test %d\:'  | wc
> >      34     222    2591
> > %
> > 
> > There's a fair weight to either side. As (I believe) the instigator of this
> > particular idiom, I prefer the "." - it's not a sentence that benefits from
a
> > colon, and it looks nice as a title.
> > 
> > If you want to go through the whole source tree standardising this, I won't
> > object, but I'll leave this as is for the time being.
> 
> Please, make it "test %d:", as you can see it's more common in the codebase
and
> I personally prefer that, as it makes more sense - it's not a sentence by
> itself, more like a prefix, hence the :. I'm not saying change every other
> occurrence, just this one.

Done.

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go
File names/tagkind.go (right):

https://codereview.appspot.com/12473043/diff/6001/names/tagkind.go#newcode28
names/tagkind.go:28: UnitTagKind:    unitFromTagName,
On 2013/08/06 10:00:28, dimitern wrote:
> On 2013/08/06 09:50:08, rog wrote:
> > On 2013/08/06 09:35:40, dimitern wrote:
> > > unitFromTag and machineFromTag please. There's no concept of a "tag name".
> > 
> > a tag has two parts - these functions operate on the second part,
> > not the whole thing as would be implied if this was called
> > unitFromTag.
> > 
> > as an arbitrary decision, i'm calling that the tag "name"
> > (as opposed to the id, as passed to UnitTag, MachineTag etc).
> > 
> > better suggestions for a name other than "name" are welcome,
> > but fromTag is wrong.
> 
> fromTagSuffix, as agreed.

Done (slightly differently)

[rog, 2013-08-06 10:19:11]

Please take a look.

[rog, 2013-08-07 11:30:27]

Please take a look.

[natefinch, 2013-08-07 12:30:17]

LGTM with just a couple very minor things.

https://codereview.appspot.com/12473043/diff/30001/names/tag.go
File names/tag.go (right):

https://codereview.appspot.com/12473043/diff/30001/names/tag.go#newcode70
names/tag.go:70: return "", "", fmt.Errorf("%q is not a valid %s tag", tag,
expectKind)
Suggest putting the invalid tag error generation in a helper function, since
it's used in 4 places, so we can make sure all uses use the same text.

https://codereview.appspot.com/12473043/diff/30001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12473043/diff/30001/state/state.go#newcode683
state/state.go:683: return nil, fmt.Errorf(`%q is not a valid environment tag`,
tag)
Could we use a different error message here? This could be confusing, since it
could be a valid environment tag, in that it's not invalid... it's just not the
current one and therefore is incorrect.

[rog, 2013-08-07 12:55:51]

Please take a look.

https://codereview.appspot.com/12473043/diff/30001/names/tag.go
File names/tag.go (right):

https://codereview.appspot.com/12473043/diff/30001/names/tag.go#newcode70
names/tag.go:70: return "", "", fmt.Errorf("%q is not a valid %s tag", tag,
expectKind)
On 2013/08/07 12:30:17, nate.finch wrote:
> Suggest putting the invalid tag error generation in a helper function, since
> it's used in 4 places, so we can make sure all uses use the same text.

That had crossed my mind too. Done.

https://codereview.appspot.com/12473043/diff/30001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/12473043/diff/30001/state/state.go#newcode683
state/state.go:683: return nil, fmt.Errorf(`%q is not a valid environment tag`,
tag)
On 2013/08/07 12:30:17, nate.finch wrote:
> Could we use a different error message here? This could be confusing, since it
> could be a valid environment tag, in that it's not invalid... it's just not
the
> current one and therefore is incorrect.

Good point. Done.

[jameinel, 2013-08-08 18:09:05]

LGTM