environs/tools: new package

environs/tools: new package

Tools selection logic implemented for `juju sync-tools` moved into its own
package, and was expanded somewhat.

https://code.launchpad.net/~fwereade/juju-core/environs-tools-list/+merge/157964

(do not edit description out of merge proposal)

[fwereade, 2013-04-10 01:04:36]

Please take a look.

[dfc, 2013-04-10 01:36:42]

LGTM. Nice cleanup.

[thumper, 2013-04-10 04:32:51]

Several comment tweaks, and one potential problem with finding newest tools. 
Obviously you are missing a test for the empty case, or this would have blown up
:-)  Also a question for filter.

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

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:11: // List is an unordered list of tools available in an
environment.
Well, since it is an array, it is an ordered list.  Perhaps we'd want to be
explicit here and say that the ordering doesn't imply importance.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:14: func (src List) String() string {
// String returns a single string value containing the string representations of
the binary version for the tools separated by semi-colons

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:43: result = append(result, value)
This is appending all keys in seen even if the corresponding value is false.  Is
this what you intended?

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:53: best := src[0].Number
What if len(src) == 0 ?

Perhaps better just to start with a version.Number?

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go
File environs/tools/list_test.go (right):

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go#...
environs/tools/list_test.go:206: }} {
This line does my head in.  I do find it easier to follow if the test structure
isn't inline in the function.

[rog, 2013-04-10 08:30:31]

very nice. lots of remarks here but only suggestions for naming and comments.
what do you think?

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

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:14: func (src List) String() string {
On 2013/04/10 04:32:51, thumper wrote:
> // String returns a single string value containing the string representations
of
> the binary version for the tools separated by semi-colons

or, simpler:
// String returns the tools in src separated by semicolons.

?

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:36: func (src List) collect(f func(map[string]bool,
*state.Tools)) []string {
i had to look twice here.

i think that the collect function might be more
immediately obvious if its function argument reflected
more directly what it is doing. then all the map-related
logic can stay together:

// collect calls f on all values in src and returns
// an alphabetically ordered list of the returned results
// without duplicates.
func (src List) collect(f func(*state.Tools) string) []string {
     seen := map[string]bool{}
     for _, tools := range src {
          seen[f(tools)] = true
     }
     result := make([]string, 0, len(seen))
     for value := range seen {
          result = append(result, value)
     }
     sort.Strings(result)
     return result
}

then:

func (src List) Series() []string {
     return src.collect(toolsSeries)
}

func toolsSeries(tools *state.Tools) string {
    return tools.Series
}

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:50: // which have a version number greater than or equal
to all other tools
// Newest returns the tools in src with the greatest version number.
?

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:53: best := src[0].Number
On 2013/04/10 04:32:51, thumper wrote:
> What if len(src) == 0 ?
> 
> Perhaps better just to start with a version.Number?

or just:
if len(src) == 0 {
    return nil
}

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:58: result = List{tools}
i missed this line the first time.
how about this?

// Found new best value; reset result list.
result = append(result[:0], tools)

that way you don't reallocate either.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:67: // binary versions not found in the supplied List.
i'm not sure that "difference" is a great word here (it doesn't imply what
difference is returned).

how about:

// Exclude returns all the tools in src that
// are not in excluded.
func (src List) Exclude(excluded List) List

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:86: func (src List) Filter(f Filter) (List, error) {
i don't think the error return is particularly helpful here. a zero-length list
is just fine, and not unexpected as a result of a filter function.

also i'm not sure that "Filter" is the best name here either - it doesn't imply
which way the filtering happens.

how about:

// Match returns all tools in src that match the supplied filter.
func (src List) Match(f Filter) List

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:99: // Filter parameterises List.Filter.
// Filter holds criteria for choosing tools.

?

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:102: // Release, if true, ignores all tools whose version
number indicates
given that we're talking about filter matching above, i'd try to phrase the
comments here in terms of matching rather than ignoring.

// Release, if true, causes the filter to match
// only tools with a non-development version.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:106: // Number, if non-zero, ignores all tools without
that version number.
// Number, if non-zero, causes the filter to match
// only tools with that version number.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:109: // Series, if not empty, ignores all tools not built
for that series.
// Series, if not empty, causes the filter to match only
// tools with that series.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:112: // Arch, if not empty, ignores all tools not built
for that architecture.
// Arch, if not empty, causes the filter to match
// only tools with that architecture.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:117: func (f Filter) match(tools *state.Tools) bool {
given that Filter is public and we talk about its matching properties, i think
it would be reasonable to expose this function publicly.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go
File environs/tools/list_test.go (right):

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go#...
environs/tools/list_test.go:206: }} {
On 2013/04/10 04:32:51, thumper wrote:
> This line does my head in.  I do find it easier to follow if the test
structure
> isn't inline in the function.

+1.
i prefer the tests to be outside the function when reasonable, as we've done
everywhere else. it saves a level of indentation, and it means that it's obvious
that none of the tests rely on dynamic information from the function's frame.

[TheMue, 2013-04-10 09:25:00]

Feels good. Some additional comments.

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

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:14: func (src List) String() string {
On 2013/04/10 08:30:32, rog wrote:
> On 2013/04/10 04:32:51, thumper wrote:
> > // String returns a single string value containing the string
representations
> of
> > the binary version for the tools separated by semi-colons
> 
> or, simpler:
> // String returns the tools in src separated by semicolons.
> 
> ?

+1 for the simple variant.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:30: func (src List) Arches() []string {
Is this a common term here, as it also has a second meaning? I would prefer
"Architectures()".

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:50: // which have a version number greater than or equal
to all other tools
On 2013/04/10 08:30:32, rog wrote:
> // Newest returns the tools in src with the greatest version number.
> ?

+1

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:86: func (src List) Filter(f Filter) (List, error) {
On 2013/04/10 08:30:32, rog wrote:
> i don't think the error return is particularly helpful here. a zero-length
list
> is just fine, and not unexpected as a result of a filter function.
> 
> also i'm not sure that "Filter" is the best name here either - it doesn't
imply
> which way the filtering happens.
> 
> how about:
> 
> // Match returns all tools in src that match the supplied filter.
> func (src List) Match(f Filter) List

I prefer Match() too. I know Filter() more from languages taking a
predicate/func to filter a list. But here we talk about a struct with matching
criteria.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go
File environs/tools/list_test.go (right):

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go#...
environs/tools/list_test.go:206: }} {
On 2013/04/10 08:30:32, rog wrote:
> On 2013/04/10 04:32:51, thumper wrote:
> > This line does my head in.  I do find it easier to follow if the test
> structure
> > isn't inline in the function.
> 
> +1.
> i prefer the tests to be outside the function when reasonable, as we've done
> everywhere else. it saves a level of indentation, and it means that it's
obvious
> that none of the tests rely on dynamic information from the function's frame.

And another +1.

[dimitern, 2013-04-10 10:03:02]

Looking good, although I agree with most other comments I have one of my own as
well.

https://codereview.appspot.com/8604043/diff/2001/cmd/juju/sync_tools.go
File cmd/juju/sync_tools.go (right):

https://codereview.appspot.com/8604043/diff/2001/cmd/juju/sync_tools.go#newcode1
cmd/juju/sync_tools.go:1: package main
Please, while you're at it, rename both files from sync_tools* to synctools* to
keep the convention with other commands.

[fwereade, 2013-04-10 23:07:29]

Please take a look.

https://codereview.appspot.com/8604043/diff/2001/cmd/juju/sync_tools.go
File cmd/juju/sync_tools.go (right):

https://codereview.appspot.com/8604043/diff/2001/cmd/juju/sync_tools.go#newcode1
cmd/juju/sync_tools.go:1: package main
On 2013/04/10 10:03:02, dimitern wrote:
> Please, while you're at it, rename both files from sync_tools* to synctools*
to
> keep the convention with other commands.

Done.

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

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:11: // List is an unordered list of tools available in an
environment.
On 2013/04/10 04:32:51, thumper wrote:
> Well, since it is an array, it is an ordered list.  Perhaps we'd want to be
> explicit here and say that the ordering doesn't imply importance.

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:14: func (src List) String() string {
On 2013/04/10 09:25:00, TheMue wrote:
> On 2013/04/10 08:30:32, rog wrote:
> > On 2013/04/10 04:32:51, thumper wrote:
> > > // String returns a single string value containing the string
> representations
> > of
> > > the binary version for the tools separated by semi-colons
> > 
> > or, simpler:
> > // String returns the tools in src separated by semicolons.
> > 
> > ?
> 
> +1 for the simple variant.

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:30: func (src List) Arches() []string {
On 2013/04/10 09:25:00, TheMue wrote:
> Is this a common term here, as it also has a second meaning? I would prefer
> "Architectures()".

It's consistent with the rest of the codebase.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:36: func (src List) collect(f func(map[string]bool,
*state.Tools)) []string {
On 2013/04/10 08:30:32, rog wrote:
> i had to look twice here.
> 
> i think that the collect function might be more
> immediately obvious if its function argument reflected
> more directly what it is doing. then all the map-related
> logic can stay together:
> 
> // collect calls f on all values in src and returns
> // an alphabetically ordered list of the returned results
> // without duplicates.
> func (src List) collect(f func(*state.Tools) string) []string {
>      seen := map[string]bool{}
>      for _, tools := range src {
>           seen[f(tools)] = true
>      }
>      result := make([]string, 0, len(seen))
>      for value := range seen {
>           result = append(result, value)
>      }
>      sort.Strings(result)
>      return result
> }
> 
> then:
> 
> func (src List) Series() []string {
>      return src.collect(toolsSeries)
> }
> 
> func toolsSeries(tools *state.Tools) string {
>     return tools.Series
> }

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:43: result = append(result, value)
On 2013/04/10 04:32:51, thumper wrote:
> This is appending all keys in seen even if the corresponding value is false. 
Is
> this what you intended?

It was; and I think it's more clearly sane given rog's suggestion now.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:50: // which have a version number greater than or equal
to all other tools
On 2013/04/10 09:25:00, TheMue wrote:
> On 2013/04/10 08:30:32, rog wrote:
> > // Newest returns the tools in src with the greatest version number.
> > ?
> 
> +1

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:53: best := src[0].Number
On 2013/04/10 08:30:32, rog wrote:
> On 2013/04/10 04:32:51, thumper wrote:
> > What if len(src) == 0 ?
> > 
> > Perhaps better just to start with a version.Number?
> 
> or just:
> if len(src) == 0 {
>     return nil
> }

My thinking was that it was a plain impossibility to get the newest tools from
an empty list, and allowing panicing was perfectly reasonable. But done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:58: result = List{tools}
On 2013/04/10 08:30:32, rog wrote:
> i missed this line the first time.
> how about this?
> 
> // Found new best value; reset result list.
> result = append(result[:0], tools)
> 
> that way you don't reallocate either.

Nice. Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:67: // binary versions not found in the supplied List.
On 2013/04/10 08:30:32, rog wrote:
> i'm not sure that "difference" is a great word here (it doesn't imply what
> difference is returned).
> 
> how about:
> 
> // Exclude returns all the tools in src that
> // are not in excluded.
> func (src List) Exclude(excluded List) List

I was vaguely thinking "set difference" when I changed it from Exclude in the
first place :). Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:86: func (src List) Filter(f Filter) (List, error) {
On 2013/04/10 09:25:00, TheMue wrote:
> On 2013/04/10 08:30:32, rog wrote:
> > i don't think the error return is particularly helpful here. a zero-length
> list
> > is just fine, and not unexpected as a result of a filter function.
> > 
> > also i'm not sure that "Filter" is the best name here either - it doesn't
> imply
> > which way the filtering happens.
> > 
> > how about:
> > 
> > // Match returns all tools in src that match the supplied filter.
> > func (src List) Match(f Filter) List
> 
> I prefer Match() too. I know Filter() more from languages taking a
> predicate/func to filter a list. But here we talk about a struct with matching
> criteria.

I'm fine with Match, but not so comfortable without err. The general idea is
that an empty List is a Bad Thing, and that any operation that could produce an
empty list from a non-empty one should signal if that's happened. The code that
uses it actually works out quite convenient with an error, I think, but I'll
keep this in mind for a followup in case it turns out to improve matters.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:99: // Filter parameterises List.Filter.
On 2013/04/10 08:30:32, rog wrote:
> // Filter holds criteria for choosing tools.
> 
> ?

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:102: // Release, if true, ignores all tools whose version
number indicates
On 2013/04/10 08:30:32, rog wrote:
> given that we're talking about filter matching above, i'd try to phrase the
> comments here in terms of matching rather than ignoring.
> 
> // Release, if true, causes the filter to match
> // only tools with a non-development version.

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:106: // Number, if non-zero, ignores all tools without
that version number.
On 2013/04/10 08:30:32, rog wrote:
> // Number, if non-zero, causes the filter to match
> // only tools with that version number.

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:109: // Series, if not empty, ignores all tools not built
for that series.
On 2013/04/10 08:30:32, rog wrote:
> // Series, if not empty, causes the filter to match only
> // tools with that series.

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:112: // Arch, if not empty, ignores all tools not built
for that architecture.
On 2013/04/10 08:30:32, rog wrote:
> // Arch, if not empty, causes the filter to match
> // only tools with that architecture.

Done.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list.go#newco...
environs/tools/list.go:117: func (f Filter) match(tools *state.Tools) bool {
On 2013/04/10 08:30:32, rog wrote:
> given that Filter is public and we talk about its matching properties, i think
> it would be reasonable to expose this function publicly.

I think I'll hold off until we need it.

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go
File environs/tools/list_test.go (right):

https://codereview.appspot.com/8604043/diff/2001/environs/tools/list_test.go#...
environs/tools/list_test.go:206: }} {
On 2013/04/10 08:30:32, rog wrote:
> On 2013/04/10 04:32:51, thumper wrote:
> > This line does my head in.  I do find it easier to follow if the test
> structure
> > isn't inline in the function.
> 
> +1.
> i prefer the tests to be outside the function when reasonable, as we've done
> everywhere else. it saves a level of indentation, and it means that it's
obvious
> that none of the tests rely on dynamic information from the function's frame.

Public opinion is clearly against me here, so I'll change it, but offer the
following counterpoints:

1) you don't get any guarantees about dynamic information affecting the tests --
that's down to the actual code in the test method, which needs to be read in
order to actually understand the test anyway.

2) by putting the tests inside the method, we *do* get guarantees that external
state can't affect the actual test structs, and that any change to the test data
will affect only this one test.

3) all these global tests get annoying in a large package. I have on several
occasions named an []test something apparently sensible only to discover that it
conflicts with some other []test nearby; so on or both gets renamed for
additional redundancy when its purpose ought to be clear from file context.

4) by making the []test itself anonymous, you we eliminate the pointless var
name entirely -- all those names ever do, apart from cause me to trip as
described above, is drift out of sync with the associated test methods and cause
confusion ;p.

5) totally subjective: come on, it's a really nice clean structure:

func (s *Suite) TestSomething(c *C) {
    for i, test := range []struct{
        // declaration
    }{{
        // definition
    }, {
        // definition
    }, {
        // definition
    }} {
        c.Logf("test %d", i)
        // code...
    }
}

...and it makes the tight scoping nicely apparent, and... ah well. Done. But if
anyone's swayed by the above and starts submitting code with anonymous []tests,
I won't complain :).

[thumper, 2013-04-10 23:39:32]

> Public opinion is clearly against me here, so I'll change it, but offer the
> following counterpoints:
> 
> 1) you don't get any guarantees about dynamic information affecting the tests
--
> that's down to the actual code in the test method, which needs to be read in
> order to actually understand the test anyway.
> 
> 2) by putting the tests inside the method, we *do* get guarantees that
external
> state can't affect the actual test structs, and that any change to the test
data
> will affect only this one test.
> 
> 3) all these global tests get annoying in a large package. I have on several
> occasions named an []test something apparently sensible only to discover that
it
> conflicts with some other []test nearby; so on or both gets renamed for
> additional redundancy when its purpose ought to be clear from file context.
> 
> 4) by making the []test itself anonymous, you we eliminate the pointless var
> name entirely -- all those names ever do, apart from cause me to trip as
> described above, is drift out of sync with the associated test methods and
cause
> confusion ;p.
> 
> 5) totally subjective: come on, it's a really nice clean structure:
> 
> func (s *Suite) TestSomething(c *C) {
>     for i, test := range []struct{
>         // declaration
>     }{{
>         // definition
>     }, {
>         // definition
>     }, {
>         // definition
>     }} {
>         c.Logf("test %d", i)
>         // code...
>     }
> }
> 
> ...and it makes the tight scoping nicely apparent, and... ah well. Done. But
if
> anyone's swayed by the above and starts submitting code with anonymous
[]tests,
> I won't complain :).

LGTM - changes look good.

You have swayed me on your inline test arrays though.

[fwereade, 2013-04-11 00:04:28]

*** Submitted:

environs/tools: new package

Tools selection logic implemented for `juju sync-tools` moved into its own
package, and was expanded somewhat.

R=dfc, thumper, rog, TheMue, dimitern
CC=
https://codereview.appspot.com/8604043