add charm.InferURL for interpretation of vaguely-sepcified charm urls

add charm.InferURL for interpretation of vaguely-sepcified charm urls



https://code.launchpad.net/~fwereade/juju/go-charm-infer-url/+merge/105606

(do not edit description out of merge proposal)

[fwereade, 2012-05-14 05:11:00]

Please take a look.

[dave_cheney.net, 2012-05-14 21:10:35]

LGTM.

[niemeyer, 2012-05-15 21:36:37]

https://codereview.appspot.com/6209062/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode113
charm/url.go:113: // where necessary, and attempts to parse the result.
// InferURL returns a charm URL inferred from src. The provided
// src may be a valid URL, in which case it is returned as-is,
// or it may be an alias in one of the following formats:
//
//    <format 1>
//    <format 2>
//    ...
//
// The defaultSeries paramater is used to define the resulting URL
// when src does not include that information.

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode131
charm/url.go:131: // construct full url string, inferring series if omitted
The comment is misleading, because the line below is not doing that. It's
constructing the full URL string, but is not inferring series.

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode132
charm/url.go:132: full := fmt.Sprintf("%s:%s", schema, rest)
The sequence of operations here is a bit hard to follow, due to the back and
forth in parsing (it potentially splits and rebuilds exactly the same thing, and
then potentially ignores that same thing rebuilt as well).

Here is a seed for a small simplification that avoids going back and forth:

schema := "cs"
if i := strings.Index(src, "/") {
    schema, src = src[:i], src[i+1:]
}
var full string
switch parts := strings.Split(src, "/"); len(parts) {
case 1:
    ...
case 2:
    ...
default:
    full = fmt.Sprintf("%s:%s", schema, src)
}

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode143
charm/url.go:143: if src != full {
if A && B { ... } <==> if A { if B { ... } }

[fwereade, 2012-05-16 05:35:21]

Please take a look.

https://codereview.appspot.com/6209062/diff/1/charm/url.go
File charm/url.go (right):

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode113
charm/url.go:113: // where necessary, and attempts to parse the result.
On 2012/05/15 21:36:37, niemeyer wrote:
> // InferURL returns a charm URL inferred from src. The provided
> // src may be a valid URL, in which case it is returned as-is,
> // or it may be an alias in one of the following formats:
> //
> //    <format 1>
> //    <format 2>
> //    ...
> //
> // The defaultSeries paramater is used to define the resulting URL
> // when src does not include that information.

Done.

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode131
charm/url.go:131: // construct full url string, inferring series if omitted
On 2012/05/15 21:36:37, niemeyer wrote:
> The comment is misleading, because the line below is not doing that. It's
> constructing the full URL string, but is not inferring series.

No, but the *block* below was... just as in the preceding comment. Stripped,
regardless.

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode132
charm/url.go:132: full := fmt.Sprintf("%s:%s", schema, rest)
On 2012/05/15 21:36:37, niemeyer wrote:
> The sequence of operations here is a bit hard to follow, due to the back and
> forth in parsing (it potentially splits and rebuilds exactly the same thing,
and
> then potentially ignores that same thing rebuilt as well).
> 
> Here is a seed for a small simplification that avoids going back and forth:
> 
> schema := "cs"
> if i := strings.Index(src, "/") {
>     schema, src = src[:i], src[i+1:]
> }
> var full string
> switch parts := strings.Split(src, "/"); len(parts) {
> case 1:
>     ...
> case 2:
>     ...
> default:
>     full = fmt.Sprintf("%s:%s", schema, src)
> }

I worry that you guys are leading me into bad habits with all this code
denormalisation ;p. Done.

https://codereview.appspot.com/6209062/diff/1/charm/url.go#newcode143
charm/url.go:143: if src != full {
On 2012/05/15 21:36:37, niemeyer wrote:
> if A && B { ... } <==> if A { if B { ... } }

Done.

[niemeyer, 2012-05-17 19:07:25]

Thanks, LGTM.

[fwereade, 2012-05-18 09:59:00]

*** Submitted:

add charm.InferURL for interpretation of vaguely-sepcified charm urls

R=dfc, niemeyer
CC=
https://codereview.appspot.com/6209062