Code review comment for lp:~cmars/juju-core/no-default-series

You have to "lbox propose" again, and then it will generate a new diff on
Reitveld, it doesn't notice plain 'bzr push' to Launchpad.

On Mon, Mar 24, 2014 at 7:57 PM, Casey Marshall <
<email address hidden>> wrote:

> I've restricted the series name to not include dashes, and resubmitted
> the MP in launchpad with the prior branch prereq:
> https://code.launchpad.net/~cmars/juju-core/cs-series-solver/+merge/212407
>
> PTAL, thanks!
>
> I'm a bit unsure whether/how lbox & rietveld will follow the LP
> resubmit..
>
> -Casey
>
>
> https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go
> File charm/url_test.go (right):
>
>
> https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode167
> charm/url_test.go:167: c.Assert(err.Error(), gc.Equals,
> charm.ErrUnresolvedUrl.Error())
> On 2014/03/24 09:24:09, fwereade wrote:
> > We'd usually check for direct equality with ErrUnresolvedUrl, and
> otherwise use
> > gc.ErrorMatches to check the string.
>
> Done.
>
>
> https://codereview.appspot.com/76860044/diff/120001/charm/url_test.go#newcode213
> charm/url_test.go:213: {charm.IsValidSeries, "pre-c1se", true},
> On 2014/03/24 09:24:09, fwereade wrote:
> > Explicit test that precise-1 is not allowed, but precise1 is?
>
> Done.
>
>
> https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go
> File state/apiserver/client/client_test.go (left):
>
>
> https://codereview.appspot.com/76860044/diff/120001/state/apiserver/client/client_test.go#oldcode663
> state/apiserver/client/client_test.go:663: // TODO(fwereade) make these
> errors consistent one day.
> On 2014/03/24 09:24:09, fwereade wrote:
> > If these errors are starting to look nicer, please drop the comment
> (and the one
> > below). If they're not, would you upgrade this to a tech-debt bug and
> fill in
> > the usual TODO template (name, date, bug#, \n, short description)?
>
> Done.
>
> https://codereview.appspot.com/76860044/diff/120001/store/server.go
> File store/server.go (right):
>
>
> https://codereview.appspot.com/76860044/diff/120001/store/server.go#newcode80
> store/server.go:80: return &charm.URL{Reference: ref, Series:
> DefaultSeries}
> On 2014/03/24 09:24:09, fwereade wrote:
> > I'm not seeing where this is called. Dead code?
>
> Yes, it is now. I've removed it.
>
> https://codereview.appspot.com/76860044/
>
> --
> https://code.launchpad.net/~cmars/juju-core/no-default-series/+merge/211389
> You are subscribed to branch lp:juju-core.
>