charm: add JSON support for URL

charm: add JSON support for URL

We want it for the API.

https://code.launchpad.net/~rogpeppe/juju-core/252-charm-url-json/+merge/154766

(do not edit description out of merge proposal)

[rog, 2013-03-21 17:32:53]

Please take a look.

[thumper, 2013-03-21 22:35:12]

https://codereview.appspot.com/7568049/diff/2001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/7568049/diff/2001/charm/url.go#newcode211
charm/url.go:211: panic("cannot marshal nil *charm.URL")
Why panic instead of returning an error?

[dave_cheney.net, 2013-03-22 00:18:21]

LGTM modulo thumper's comment.

https://codereview.appspot.com/7568049/diff/2001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/7568049/diff/2001/charm/url.go#newcode211
charm/url.go:211: panic("cannot marshal nil *charm.URL")
On 2013/03/21 22:35:12, thumper wrote:
> Why panic instead of returning an error?

+1 to returning an error. Is that case covered in the tests ?

[fwereade, 2013-03-22 00:54:55]

IMO we should forget the whole error/panic thing in MarshalJSON, and just let
the panics happen when they may (which should be never, barring surprising
behaviour in the json package; and we probably shouldn't be writing code to
explicitly handle the possibility that some dependency suddenly starts working
differently).

If my conception of why a panic is appropriate makes sense, LGTM with
appropriate action; otherwise we should maybe chat. dfc, thumper, please shout
if you think this is crazy.

https://codereview.appspot.com/7568049/diff/2001/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/7568049/diff/2001/charm/url.go#newcode211
charm/url.go:211: panic("cannot marshal nil *charm.URL")
On 2013/03/22 00:18:21, dfc wrote:
> On 2013/03/21 22:35:12, thumper wrote:
> > Why panic instead of returning an error?
> 
> +1 to returning an error. Is that case covered in the tests ?

I think that if the serialization package is trying to marshal a nil pointer by
calling methods on it, it's reasonable to throw a tantrum.

However, I'm not sure the explicit panic is a big win over just letting the
nil-dereference in the next line do it for us: if that does ever happen, the
context should be pretty clear.

[dave_cheney.net, 2013-03-22 00:57:29]

No need to have a chat. Just remove the nil check, this isn't that important.

On Fri, Mar 22, 2013 at 11:54 AM,  <fwereade@gmail.com> wrote:
> IMO we should forget the whole error/panic thing in MarshalJSON, and
> just let the panics happen when they may (which should be never, barring
> surprising behaviour in the json package; and we probably shouldn't be
> writing code to explicitly handle the possibility that some dependency
> suddenly starts working differently).
>
> If my conception of why a panic is appropriate makes sense, LGTM with
> appropriate action; otherwise we should maybe chat. dfc, thumper, please
> shout if you think this is crazy.
>
>
>
> https://codereview.appspot.com/7568049/diff/2001/charm/url.go
> File charm/url.go (right):
>
> https://codereview.appspot.com/7568049/diff/2001/charm/url.go#newcode211
> charm/url.go:211: panic("cannot marshal nil *charm.URL")
> On 2013/03/22 00:18:21, dfc wrote:
>>
>> On 2013/03/21 22:35:12, thumper wrote:
>> > Why panic instead of returning an error?
>
>
>> +1 to returning an error. Is that case covered in the tests ?
>
>
> I think that if the serialization package is trying to marshal a nil
> pointer by calling methods on it, it's reasonable to throw a tantrum.
>
> However, I'm not sure the explicit panic is a big win over just letting
> the nil-dereference in the next line do it for us: if that does ever
> happen, the context should be pretty clear.
>
> https://codereview.appspot.com/7568049/

[rog, 2013-03-22 16:57:18]

*** Submitted:

charm: add JSON support for URL

We want it for the API.

R=thumper, dfc, fwereade
CC=
https://codereview.appspot.com/7568049