Use the new set.Strings

Use the new set.Strings

Update some other call-sites that were using map[string]bool when they really
wanted sets of strings.

https://code.launchpad.net/~thumper/juju-core/use-string-set/+merge/158531

Requires: https://code.launchpad.net/~thumper/juju-core/string-set/+merge/158530

(do not edit description out of merge proposal)

[thumper, 2013-04-12 05:24:41]

Please take a look.

[fwereade, 2013-04-12 15:13:52]

LGTM, but you might also want to cast an eye over the code for
`map[string]struct{}`s -- if there are any, they'd make good StringSets too.

[thumper, 2013-04-15 00:05:56]

Please take a look.

[dfc, 2013-04-15 00:22:24]

Very close, some minor comments, including ones that I should have picked up in
your previous CL if I hadn't glibly waved it through.

https://codereview.appspot.com/8701043/diff/4001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/8701043/diff/4001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:44: if ctx.deployed.Size() == 0 {
suggestion, add an Empty() or IsEmpty() method to set.Strings ?

https://codereview.appspot.com/8701043/diff/4001/environs/tools/list.go
File environs/tools/list.go (right):

https://codereview.appspot.com/8701043/diff/4001/environs/tools/list.go#newco...
environs/tools/list.go:40: seen := set.NewStrings()
var seen set.Strings

https://codereview.appspot.com/8701043/diff/4001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/8701043/diff/4001/state/watcher.go#newcode638
state/watcher.go:638: for _, key := range w.watching.Values() {
keys go in the key position, values go in the value position. It's unfortunate
that the name of this value is `key`

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go
File utils/set/stringset.go (right):

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go#newco...
utils/set/stringset.go:50: _, exists := s.values[value]
return s.values[value]

if the key is not present in the set, then the zero value for the map value will
be returned, which is false.

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go#newco...
utils/set/stringset.go:75: result := NewStrings()
var result Strings

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go#newco...
utils/set/stringset.go:90: result := NewStrings()
var result Strings

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go#newco...
utils/set/stringset.go:104: result := NewStrings()
var result Strings

https://codereview.appspot.com/8701043/diff/4001/worker/uniter/jujuc/ports_te...
File worker/uniter/jujuc/ports_test.go (right):

https://codereview.appspot.com/8701043/diff/4001/worker/uniter/jujuc/ports_te...
worker/uniter/jujuc/ports_test.go:21: {[]string{"open-port", "80"},
set.NewStrings("80/tcp")},
nice.

[thumper, 2013-04-15 01:05:50]

Please take a look.

https://codereview.appspot.com/8701043/diff/4001/cmd/jujud/deploy_test.go
File cmd/jujud/deploy_test.go (right):

https://codereview.appspot.com/8701043/diff/4001/cmd/jujud/deploy_test.go#new...
cmd/jujud/deploy_test.go:44: if ctx.deployed.Size() == 0 {
On 2013/04/15 00:22:24, dfc wrote:
> suggestion, add an Empty() or IsEmpty() method to set.Strings ?

Added IsEmpty()

https://codereview.appspot.com/8701043/diff/4001/environs/tools/list.go
File environs/tools/list.go (right):

https://codereview.appspot.com/8701043/diff/4001/environs/tools/list.go#newco...
environs/tools/list.go:40: seen := set.NewStrings()
On 2013/04/15 00:22:24, dfc wrote:
> var seen set.Strings

Done.

https://codereview.appspot.com/8701043/diff/4001/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/8701043/diff/4001/state/watcher.go#newcode638
state/watcher.go:638: for _, key := range w.watching.Values() {
On 2013/04/15 00:22:24, dfc wrote:
> keys go in the key position, values go in the value position. It's unfortunate
> that the name of this value is `key`

s/key/watchedValue/

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go
File utils/set/stringset.go (right):

https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go#newco...
utils/set/stringset.go:75: result := NewStrings()
On 2013/04/15 00:22:24, dfc wrote:
> var result Strings

As discussed on IRC, using NewStrings() here means we can poke the
implementation knowing that it has been initialized.

[dfc, 2013-04-15 01:28:14]

LGTM. Thank you. 

On 15/04/2013, at 11:05, tim.penhey@canonical.com wrote:

> Please take a look.
> 
> 
> https://codereview.appspot.com/8701043/diff/4001/cmd/jujud/deploy_test.go
> File cmd/jujud/deploy_test.go (right):
> 
>
https://codereview.appspot.com/8701043/diff/4001/cmd/jujud/deploy_test.go#new...
> cmd/jujud/deploy_test.go:44: if ctx.deployed.Size() == 0 {
> On 2013/04/15 00:22:24, dfc wrote:
>> suggestion, add an Empty() or IsEmpty() method to set.Strings ?
> 
> Added IsEmpty()
> 
> https://codereview.appspot.com/8701043/diff/4001/environs/tools/list.go
> File environs/tools/list.go (right):
> 
>
https://codereview.appspot.com/8701043/diff/4001/environs/tools/list.go#newco...
> environs/tools/list.go:40: seen := set.NewStrings()
> On 2013/04/15 00:22:24, dfc wrote:
>> var seen set.Strings
> 
> Done.
> 
> https://codereview.appspot.com/8701043/diff/4001/state/watcher.go
> File state/watcher.go (right):
> 
> https://codereview.appspot.com/8701043/diff/4001/state/watcher.go#newcode638
> state/watcher.go:638: for _, key := range w.watching.Values() {
> On 2013/04/15 00:22:24, dfc wrote:
>> keys go in the key position, values go in the value position. It's
> unfortunate
>> that the name of this value is `key`
> 
> s/key/watchedValue/
> 
> https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go
> File utils/set/stringset.go (right):
> 
>
https://codereview.appspot.com/8701043/diff/4001/utils/set/stringset.go#newco...
> utils/set/stringset.go:75: result := NewStrings()
> On 2013/04/15 00:22:24, dfc wrote:
>> var result Strings
> 
> As discussed on IRC, using NewStrings() here means we can poke the
> implementation knowing that it has been initialized.
> 
> https://codereview.appspot.com/8701043/

[thumper, 2013-04-15 01:35:40]

*** Submitted:

Use the new set.Strings

Update some other call-sites that were using map[string]bool when they really
wanted sets of strings.

R=fwereade, dfc
CC=
https://codereview.appspot.com/8701043