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

Revision history for this message
Casey Marshall (cmars) 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/

« Back to merge proposal