Fix megawatcher to also unescape settings

Fix megawatcher to also unescape settings

Per rog's review, cleaned up a few things here and there and also fix
megawatcher to unescape properly.

https://code.launchpad.net/~sidnei/juju-core/encode-dollar-and-dot-2/+merge/177854

(do not edit description out of merge proposal)

[sidnei.da.silva, 2013-07-31 14:17:13]

Please take a look.

[rog, 2013-07-31 15:47:47]

LGTM with some thoughts below

https://codereview.appspot.com/12168043/diff/1/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode19
state/settings.go:19: var fullWidthDot = "\uff0e"
perhaps put these all in a

var (
   ...
)

block.

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode116
state/settings.go:116: // copyMap copies the keys and values of one map into a
new one.
// If transform is non-nil, for each old key k,
// the new key will be transform(k).

?

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode246
state/settings.go:246: replaceKeys(config, unescapeReplacer)
can't we put this replaceKeys call inside cleanSettingsMap? then megawatcher.go
won't need to know another piece of settings internals.

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode262
state/settings.go:262: newValues := copyMap(values, func(s string) string {
newValues := copyMap(values, escapeReplacer.Replace)

'cos we can use go1.1 features now.

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode312
state/settings.go:312: newValues := copyMap(values, func(s string) string {
ditto

[dimitern, 2013-07-31 16:06:19]

LGTM with a few trivial suggestions.

https://codereview.appspot.com/12168043/diff/1/state/megawatcher_internal_tes...
File state/megawatcher_internal_test.go (right):

https://codereview.appspot.com/12168043/diff/1/state/megawatcher_internal_tes...
state/megawatcher_internal_test.go:794: AddCustomCharm(c, st, "wordpress",
"config.yaml",
Please, put this on a separate line before st.AddService (it's a bit hard to
read it now):
testCh := AddCustomCharm(
  c, st, "wordpress",
  "config.yaml", dottedConfig,
  "series", 3
)
And then:
svc, err := st.AddService("wordpress", testCh)

https://codereview.appspot.com/12168043/diff/1/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode19
state/settings.go:19: var fullWidthDot = "\uff0e"

I'd prefer here something like this:

const (
    fullWidthDot = "\uff0e"
    fullWidthDollar = "\uff04"
)

var (
    escapeReplacer = strings.NewReplacer(".", fullWidthDot, "$",
fullWidthDollar)
    unescapeReplacer = strings.NewReplacer(fullWidthDot, ".", fullWidthDollar,
"$")
)

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode116
state/settings.go:116: // copyMap copies the keys and values of one map into a
new one.
On 2013/07/31 15:47:47, rog wrote:
> // If transform is non-nil, for each old key k,
> // the new key will be transform(k).
> 
> ?

+1

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode208
state/settings.go:208: // Given a strings.Replacer, modify the provided map in
place by replacing keys
// replaceKeys, given a ...

(doc comments should start with the function name)

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode262
state/settings.go:262: newValues := copyMap(values, func(s string) string {
On 2013/07/31 15:47:47, rog wrote:
> newValues := copyMap(values, escapeReplacer.Replace)
> 
> 'cos we can use go1.1 features now.

+1 Method expressions

[rog, 2013-07-31 16:09:31]

https://codereview.appspot.com/12168043/diff/1/state/settings.go
File state/settings.go (right):

https://codereview.appspot.com/12168043/diff/1/state/settings.go#newcode19
state/settings.go:19: var fullWidthDot = "\uff0e"
On 2013/07/31 16:06:19, dimitern wrote:
> 
> I'd prefer here something like this:
> 
> const (
>     fullWidthDot = "\uff0e"
>     fullWidthDollar = "\uff04"
> )
> 
> var (
>     escapeReplacer = strings.NewReplacer(".", fullWidthDot, "$",
> fullWidthDollar)
>     unescapeReplacer = strings.NewReplacer(fullWidthDot, ".", fullWidthDollar,
> "$")
> )

+1

[sidnei.da.silva, 2013-07-31 17:39:54]

Please take a look.

[rog, 2013-07-31 17:50:09]

LGTM with a few final suggestions.

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go
File state/settings_test.go (right):

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go#newc...
state/settings_test.go:29: func cleanMgoSettings(in map[string]interface{}) {
i'd put this at the end of the file - it's not important enough to be at the
start.

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go#newc...
state/settings_test.go:130: cleanMgoSettings(mgoData)
cleanSettingsMap is still appropriate here, i think, because options (which
we're comparing against) is used to call Update, which expects unescaped values.

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go#newc...
state/settings_test.go:212: cleanMgoSettings(mgoData)
similarly here

[sidnei.da.silva, 2013-07-31 18:56:37]

Please take a look.

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go
File state/settings_test.go (right):

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go#newc...
state/settings_test.go:29: func cleanMgoSettings(in map[string]interface{}) {
On 2013/07/31 17:50:10, rog wrote:
> i'd put this at the end of the file - it's not important enough to be at the
> start.

Done.

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go#newc...
state/settings_test.go:130: cleanMgoSettings(mgoData)
On 2013/07/31 17:50:10, rog wrote:
> cleanSettingsMap is still appropriate here, i think, because options (which
> we're comparing against) is used to call Update, which expects unescaped
values.

It is actually appropriate because options doesn't contain dot or dollar here.
Done.

https://codereview.appspot.com/12168043/diff/9001/state/settings_test.go#newc...
state/settings_test.go:212: cleanMgoSettings(mgoData)
On 2013/07/31 17:50:10, rog wrote:
> similarly here

Same as above, done.