Code review comment for lp:~thumper/juju-core/support-string-slice-args

Revision history for this message
Roger Peppe (rogpeppe) wrote :

Looks reasonable - some suggestions below.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go
File cmd/args.go (right):

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode15
cmd/args.go:15: type StringsValue struct {
why not:

type StringsValue []string

?

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode21
cmd/args.go:21: func NewStringsValue(defaultValue []string, target
*[]string) *StringsValue {
If the type is as above, this function has less value.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode31
cmd/args.go:31: trimmed := strings.TrimSpace(v)
I'm not sure I'd do this - I think it's not a bad thing to be able to
include spaces.

If you do decide it's necessary, this behaviour should at least be
documented.

https://codereview.appspot.com/39150045/diff/1/cmd/args.go#newcode32
cmd/args.go:32: if trimmed != "" {
I definitely wouldn't do this.
It's perfectly reasonable to want an empty item in a list.

https://codereview.appspot.com/39150045/

« Back to merge proposal