store: added CharmEventKind.String

String representation is useful in logs, etc.

https://code.launchpad.net/~niemeyer/juju/go-event-strings/+merge/95498

(do not edit description out of merge proposal)

[niemeyer, 2012-03-02 02:15:28]

Please take a look.

[rog, 2012-03-02 09:05:36]

LGTM modulo the below

https://codereview.appspot.com/5722045/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5722045/diff/1/store/store.go#newcode500
store/store.go:500: panic("unreachable")
it's not actually unreachable - this will happen any time this method is called
on a zero CharmEventKind, for example.

i'd either panic with fmt.Errorf("unknown charm event kind %d", k) or just
return fmt.Sprintf("unknown-charm-event-%d", k) depending on how severe you rate
this error.

"unreachable" panics should be kept for genuinely unreachable code IMO.

[fwereade, 2012-03-02 16:59:50]

LGTM

https://codereview.appspot.com/5722045/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5722045/diff/1/store/store.go#newcode500
store/store.go:500: panic("unreachable")
On 2012/03/02 09:05:36, rog wrote:
> it's not actually unreachable - this will happen any time this method is
called
> on a zero CharmEventKind, for example.
> 
> i'd either panic with fmt.Errorf("unknown charm event kind %d", k) or just
> return fmt.Sprintf("unknown-charm-event-%d", k) depending on how severe you
rate
> this error.
> 
> "unreachable" panics should be kept for genuinely unreachable code IMO.

I dunno, that would seem a touch pointless :p. To me "unreachable" implies "of
course it's reachable, but if you actually reach it something's gone badly
wrong"; and this seems like one of those cases to me (especially since the test
case should induce this pretty reliably (right?)).

[niemeyer, 2012-03-02 17:25:58]

https://codereview.appspot.com/5722045/diff/1/store/store.go
File store/store.go (right):

https://codereview.appspot.com/5722045/diff/1/store/store.go#newcode500
store/store.go:500: panic("unreachable")
On 2012/03/02 16:59:51, fwereade wrote:
> I dunno, that would seem a touch pointless :p. To me "unreachable" implies "of
> course it's reachable, but if you actually reach it something's gone badly
> wrong"; and this seems like one of those cases to me (especially since the
test
> case should induce this pretty reliably (right?)).

I actually agree with rog. It's trivial to reach that point in the code.
Unreachable should mean it's simply not feasible to touch that logic unless
there's a bug *here*. That's not the case. It's trivial to go past the switch
with normal Go code. It'll still be a panic, but I'll fix the message.

[niemeyer, 2012-03-02 18:23:46]

*** Submitted:

store: added CharmEventKind.String

String representation is useful in logs, etc.

R=rog, fwereade
CC=
https://codereview.appspot.com/5722045