Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(815)

Issue 94060044: cmd/juju: refactor --series to use StringsValue

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 8 months ago by menn0
Modified:
5 years, 8 months ago
Reviewers:
fwereade, mp+218152, thumper
Visibility:
Public.

Description

cmd/juju: refactor --series to use StringsValue seriesVar is now called seriesValue and is implemented using StringsValue. This reuses StringValue while preserving the series validation logic. https://code.launchpad.net/~menno.smits/juju-core/1302005-bootstrap_series/+merge/218152 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 1

Patch Set 2 : cmd/juju: refactor --series to use StringsValue #

Patch Set 3 : cmd/juju: refactor --series to use StringsValue #

Total comments: 10
Unified diffs Side-by-side diffs Delta from patch set Stats (+106 lines, -5 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/bootstrap.go View 1 2 6 chunks +47 lines, -4 lines 7 comments Download
M cmd/juju/bootstrap_test.go View 1 2 4 chunks +57 lines, -1 line 3 comments Download

Messages

Total messages: 9
menn0
Please take a look.
5 years, 8 months ago (2014-05-02 20:38:12 UTC) #1
fwereade
LGTM https://codereview.appspot.com/94060044/diff/1/cmd/juju/bootstrap.go File cmd/juju/bootstrap.go (right): https://codereview.appspot.com/94060044/diff/1/cmd/juju/bootstrap.go#newcode128 cmd/juju/bootstrap.go:128: } Quibble quibble whine: I'd rather validate before ...
5 years, 8 months ago (2014-05-06 07:37:55 UTC) #2
menn0
Please take a look.
5 years, 8 months ago (2014-05-06 23:05:20 UTC) #3
menn0
Please take a look.
5 years, 8 months ago (2014-05-08 01:35:18 UTC) #4
thumper
On 2014/05/08 01:35:18, menn0 wrote: > Please take a look. Firstly, please don't reuse branches. ...
5 years, 8 months ago (2014-05-08 01:41:19 UTC) #5
thumper
LGTM with a few minor comments. https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go File cmd/juju/bootstrap.go (right): https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode85 cmd/juju/bootstrap.go:85: f.Var(newSeriesValue(nil, &c.seriesOld), "series", ...
5 years, 8 months ago (2014-05-08 01:53:05 UTC) #6
menn0
https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go File cmd/juju/bootstrap.go (right): https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode85 cmd/juju/bootstrap.go:85: f.Var(newSeriesValue(nil, &c.seriesOld), "series", "upload tools for supplied comma-separated series ...
5 years, 8 months ago (2014-05-08 02:35:48 UTC) #7
thumper
https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go File cmd/juju/bootstrap.go (right): https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newcode85 cmd/juju/bootstrap.go:85: f.Var(newSeriesValue(nil, &c.seriesOld), "series", "upload tools for supplied comma-separated series ...
5 years, 8 months ago (2014-05-08 04:53:52 UTC) #8
menn0
5 years, 8 months ago (2014-05-08 05:20:48 UTC) #9
https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go
File cmd/juju/bootstrap.go (right):

https://codereview.appspot.com/94060044/diff/40001/cmd/juju/bootstrap.go#newc...
cmd/juju/bootstrap.go:174: _bootstrap := getBootstrapFuncs()
On 2014/05/08 04:53:52, thumper wrote:
> On 2014/05/08 02:35:48, menn0 wrote:
> > On 2014/05/08 01:53:05, thumper wrote:
> > > a small part of me recoils at this prefix, but I can't really think of a
> > better
> > > one that makes more sense, so ok with it.
> > 
> > I didn't like this much either. Alternatives I also considered where:
> > 
> > * bootstrap (shadowing the module): potentially confusing for the casual
> reader.
> > * bootstrapApi: but it's not really an official API of any sort
> > * bootstrapFuncs or similar: collides with the struct but could change the
> > struct name.
> > 
> > Do any of these appeal more than _bootstrap?
> 
> Shadowing the outer struct shouldn't be a big issue. I think of those options,
> I'd prefer
>   bootstrapFuncs
> 
> I know others would prefer "bf" or similar, but not me.

The only (minor) I have with bootstrapFuncs is that it's a type in the outer
scope and an instance of the type inside. Changing it anyway...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b