Expanded variables

environs/config: expand field names https://code.launchpad.net/~waigani/juju-core/expand-config-variable-names/+merge/204412 (do not edit description out of merge proposal)

[waigani, 2014-02-02 23:24:02]

Please take a look.

[wallyworld, 2014-02-03 01:03:35]

LGTM but with the issues highlighted fixed. https://codereview.appspot.com/59520044/diff/1/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:633: definedAttrs := c.UnknownAttrs() This one doesn't need to be renamed - here 'm' is just a name of a map. Actually, maybe rename to 'allAttrs' https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:642: definedAttrs := c.AllAttrs() Rename 'm' to 'newAttrs' here

[rog, 2014-02-03 08:39:54]

LGTM with one suggestion below. When making a change that affects a single package, please put the package name at the start of the merge proposal description, so the scope of the change is obvious in the commit log. It's also generally good to use present rather than past tense. In this case, I'd suggest something like: environs/config: expand field names https://codereview.appspot.com/59520044/diff/1/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:68: definedAttrs, unknownAttrs map[string]interface{} It's nice to make these field names a little more obvious, but I think the "Attrs" suffix is perhaps a little more than necessary. How about "defined" and "unknown" respectively? The fact that they're in the config struct should be enough to provide the "attribute"ness.

[waigani, 2014-02-03 20:57:51]

https://codereview.appspot.com/59520044/diff/1/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:68: definedAttrs, unknownAttrs map[string]interface{} On 2014/02/03 08:39:54, rog wrote: > It's nice to make these field names a little more obvious, but I think the > "Attrs" suffix is perhaps a little more than necessary. > > How about "defined" and "unknown" respectively? The fact that they're in the > config struct should be enough to provide the "attribute"ness. > line 623: type Config has both field and method named unknown. Suggestions? https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:633: definedAttrs := c.UnknownAttrs() On 2014/02/03 01:03:36, wallyworld wrote: > This one doesn't need to be renamed - here 'm' is just a name of a map. > Actually, maybe rename to 'allAttrs' Done. https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:642: definedAttrs := c.AllAttrs() On 2014/02/03 01:03:36, wallyworld wrote: > Rename 'm' to 'newAttrs' here Done.

[rog, 2014-02-04 14:02:42]

LGTM, thanks, assuming you change the merge-proposal description as suggested. (you can do that with lbox propose -edit) https://codereview.appspot.com/59520044/diff/1/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... environs/config/config.go:68: definedAttrs, unknownAttrs map[string]interface{} On 2014/02/03 20:57:52, waigani wrote: > On 2014/02/03 08:39:54, rog wrote: > > It's nice to make these field names a little more obvious, but I think the > > "Attrs" suffix is perhaps a little more than necessary. > > > > How about "defined" and "unknown" respectively? The fact that they're in the > > config struct should be enough to provide the "attribute"ness. > > > > line 623: type Config has both field and method named unknown. Suggestions? I'm not quite sure why you're getting that error message - AFAICS there's no method named "unknown".

[waigani, 2014-02-04 19:33:57]

On 2014/02/04 14:02:42, rog wrote: > LGTM, thanks, assuming you change the merge-proposal > description as suggested. > (you can do that with lbox propose -edit) > > https://codereview.appspot.com/59520044/diff/1/environs/config/config.go > File environs/config/config.go (right): > > https://codereview.appspot.com/59520044/diff/1/environs/config/config.go#newc... > environs/config/config.go:68: definedAttrs, unknownAttrs map[string]interface{} > On 2014/02/03 20:57:52, waigani wrote: > > On 2014/02/03 08:39:54, rog wrote: > > > It's nice to make these field names a little more obvious, but I think the > > > "Attrs" suffix is perhaps a little more than necessary. > > > > > > How about "defined" and "unknown" respectively? The fact that they're in the > > > config struct should be enough to provide the "attribute"ness. > > > > > > > line 623: type Config has both field and method named unknown. Suggestions? > > I'm not quite sure why you're getting that error message - AFAICS > there's no method named "unknown". When I try to update the lbox description, I get the following error: lbox propose -edit Branches: lp:~waigani/juju-core/expand-config-variable-names => lp:juju-core Proposal: https://code.launchpad.net/~waigani/juju-core/expand-config-variable-names/+m... ----- environs/config: expand field names ----- error: Failed to send patch set to codereview: diff is empty