nova: Refactor Filter

nova: Refactor Filter

Now nova client and test double use separate filter types for better
isolation and simple code: nova.Filter (by the client), having only
Set() as it's write-only; and novaservice.filter (by the double), being
a thin wrapper over map[string]string.

As discussed, this relays the intent better and makes code cleaner.
Also removed a now redundant test to compare Add() and Set() on nova.Filter
and updated all tests as needed.

https://code.launchpad.net/~dimitern/goose/nova-filter-refactoring/+merge/147371

(do not edit description out of merge proposal)

[dimitern, 2013-02-08 14:11:43]

Please take a look.

[rog, 2013-02-08 14:45:45]

looks reasonable, but i think we make things simpler.

https://codereview.appspot.com/7301064/diff/1/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7301064/diff/1/nova/nova.go#newcode90
nova/nova.go:90: func (f *Filter) Get(filter string) string {
do we really need any methods other than Set?
have we got anything (outside of nova itself) that needs to interrogate a
filter?

https://codereview.appspot.com/7301064/diff/1/nova/nova.go#newcode158
nova/nova.go:158: requestData := goosehttp.RequestData{RespValue: &resp, Params:
filter.AsValues(), ExpectedStatus: []int{http.StatusOK}}
s/AsValues()/v/ ?

no real need for the AsValues method AFAICS, let alone for it to be exported.

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
testservices/novaservice/service_http.go:754: filter.Set(filterKey, value)
this feels much cleaner

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
File testservices/novaservice/service_http_test.go (right):

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
testservices/novaservice/service_http_test.go:782: c.Assert(expected.Servers[0],
DeepEquals, servers[0])
why has this changed?

[rog, 2013-02-08 14:46:13]

On 2013/02/08 14:45:45, rog wrote:
> looks reasonable, but i think we make things simpler.

s/we make/we can make/
:-)

[dimitern, 2013-02-08 15:09:48]

Please take a look.

https://codereview.appspot.com/7301064/diff/1/nova/nova.go
File nova/nova.go (right):

https://codereview.appspot.com/7301064/diff/1/nova/nova.go#newcode90
nova/nova.go:90: func (f *Filter) Get(filter string) string {
On 2013/02/08 14:45:45, rog wrote:
> do we really need any methods other than Set?
> have we got anything (outside of nova itself) that needs to interrogate a
> filter?

Yes, inside the nova double, which is in a different package and uses the filter
to actually perform the filtering.

https://codereview.appspot.com/7301064/diff/1/nova/nova.go#newcode158
nova/nova.go:158: requestData := goosehttp.RequestData{RespValue: &resp, Params:
filter.AsValues(), ExpectedStatus: []int{http.StatusOK}}
On 2013/02/08 14:45:45, rog wrote:
> s/AsValues()/v/ ?
> 
> no real need for the AsValues method AFAICS, let alone for it to be exported.

I agree, sure, I'll remove this.

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
File testservices/novaservice/service_http.go (right):

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
testservices/novaservice/service_http.go:754: filter.Set(filterKey, value)
On 2013/02/08 14:45:45, rog wrote:
> this feels much cleaner

Yes, I realized I missed this one before (only did the servers case, not servers
detail).

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
File testservices/novaservice/service_http_test.go (right):

https://codereview.appspot.com/7301064/diff/1/testservices/novaservice/servic...
testservices/novaservice/service_http_test.go:782: c.Assert(expected.Servers[0],
DeepEquals, servers[0])
On 2013/02/08 14:45:45, rog wrote:
> why has this changed?

Because of the ParseForm / assign stuff. Since I'm using Get(), url.Values.Get()
returns the first value set, and not using Set() in a loop there causes
name=X&name=Y to populate  the values in order, so Get(name) = X instead of Y,
as it should be. This caused this test to fail, after fixing
handleServersDetail(), hence the change.

[dimitern, 2013-02-08 16:53:57]

Please take a look.

[dimitern, 2013-02-08 16:57:22]

Please take a look.

[rog, 2013-02-08 17:02:18]

one final suggestion.

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
File testservices/novaservice/service.go (right):

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
testservices/novaservice/service.go:243: func newFilter() *filter {
why bother with the methods?
if you delete this function and the methods, you end up with a simple map, which
seems ok to me. you could keep the type though.

given that it's an internal type, i'm not sure the extra level of abstraction is
a win.

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
testservices/novaservice/service.go:267: // f := newFilter()
f := filter{
    nova.FilterStatus: nova.StatusActive,
    nova.FilterServer: `foo.*`,
}

?

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
File testservices/novaservice/service_test.go (right):

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
testservices/novaservice/service_test.go:291: f = newFilter()
much clearer, thanks.

[dimitern, 2013-02-08 17:23:50]

Please take a look.

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
File testservices/novaservice/service.go (right):

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
testservices/novaservice/service.go:267: // f := newFilter()
On 2013/02/08 17:02:18, rog wrote:
> f := filter{
>     nova.FilterStatus: nova.StatusActive,
>     nova.FilterServer: `foo.*`,
> }
> 
> ?

Great! Wasn't sure it's possible - but still learning new things about maps and
go :)
Done!

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
File testservices/novaservice/service_test.go (right):

https://codereview.appspot.com/7301064/diff/8001/testservices/novaservice/ser...
testservices/novaservice/service_test.go:291: f = newFilter()
On 2013/02/08 17:02:18, rog wrote:
> much clearer, thanks.

Yes, I agree.

[rog, 2013-02-08 17:28:43]

LGTM

[dimitern, 2013-02-08 17:36:46]

**** Submitted through the bot