Code review comment for lp:~themue/juju-core/037-empty-strings-in-charm-config

Revision history for this message
William Reade (fwereade) wrote :

Sorry, NOT LGTM without explanation. The API has lost/changed
functionality, I think, because we can no longer set string values to
defaults; and we've now got two ways to set defaults for non-string
types. How is all this going to work in the imminent CLI-API world?

https://codereview.appspot.com/12343045/diff/1/charm/config.go
File charm/config.go (right):

https://codereview.appspot.com/12343045/diff/1/charm/config.go#newcode39
charm/config.go:39: // are always considered valid, and empty string
values are converted to nil.
This isn't true any more.

https://codereview.appspot.com/12343045/diff/1/charm/config.go#newcode48
charm/config.go:48: } else if option.Type != "string" && value == "" {
Why are we keeping this behaviour? Having `value=` mean different things
depending on type seems like a bad idea to me. Can't we just delete this
whole special-case block?

https://codereview.appspot.com/12343045/diff/1/charm/config.go#newcode67
charm/config.go:67: if option.Type != "string" && str == "" {
Similarly.

https://codereview.appspot.com/12343045/diff/1/charm/config_test.go
File charm/config_test.go (right):

https://codereview.appspot.com/12343045/diff/1/charm/config_test.go#newcode337
charm/config_test.go:337: info: "empty strings are nil values for
non-string types",
Yeah -- why do we now have two ways of specifying set-to-default? I
don't think this is a good idea *unless* we have to support `value=` for
python compatibility.

https://codereview.appspot.com/12343045/diff/1/state/statecmd/config_test.go
File state/statecmd/config_test.go (right):

https://codereview.appspot.com/12343045/diff/1/state/statecmd/config_test.go#newcode90
state/statecmd/config_test.go:90: "value": "",
So how do we set defaults over the API?

https://codereview.appspot.com/12343045/

« Back to merge proposal