testing/filetesting: new package

testing/filetesting: new package

...extracted from utils/zip, and given some actual tests, which (surprise
surprise) made a few problems apparent.

https://code.launchpad.net/~fwereade/juju-core/filetesting-package/+merge/212811

(do not edit description out of merge proposal)

[fwereade, 2014-03-26 09:59:56]

Please take a look.

[fwereade, 2014-03-27 09:18:20]

Please take a look.

[rog, 2014-03-27 09:39:24]

Looks nice; a few comments and suggestions below.

https://codereview.appspot.com/80540043/diff/1/testing/filetesting/filetestin...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/1/testing/filetesting/filetestin...
testing/filetesting/filetesting.go:26: var _ Entry = Dir{}
group these into a () block?

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:12: gc "launchpad.net/gocheck"
This should be in a different section.

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:21: GetPath() string
This irks me (I'd like to name it Path), but I can't think of a better name for
the Path fields.

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:24: // a copy of the receiver.
What's the point of returning a copy of the receiver? all your implementations
are by-value anyway, so it's really a no-op.

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:46: func (e Entries) Create(c *gc.C, basePath
string) Entries {
I don't really see the point of returning a copy of the entries here (or in
Entries.Check).

If you really find yourself copying entry slices a lot, then

func CloneEntries(e Entries) Entries {
    return append(Entries{}, e...)
}

should do the job, but without seeing code that's using
these primitives, it's difficult to tell.

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:65: func (e Entries) Removeds() Entries {
AsRemoved ?
(as it is, it sounds a bit like it's returning those
entries that are instances of Removed)

[fwereade, 2014-04-02 08:42:53]

Please take a look.

https://codereview.appspot.com/80540043/diff/1/testing/filetesting/filetestin...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/1/testing/filetesting/filetestin...
testing/filetesting/filetesting.go:26: var _ Entry = Dir{}
On 2014/03/27 09:39:24, rog wrote:
> group these into a () block?

Done.

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:12: gc "launchpad.net/gocheck"
On 2014/03/27 09:39:24, rog wrote:
> This should be in a different section.

Really? They're both external packages...

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:21: GetPath() string
On 2014/03/27 09:39:24, rog wrote:
> This irks me (I'd like to name it Path), but I can't think of a better name
for
> the Path fields.

Yeah, I know :).

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:24: // a copy of the receiver.
On 2014/03/27 09:39:24, rog wrote:
> What's the point of returning a copy of the receiver? all your implementations
> are by-value anyway, so it's really a no-op.

Compactness in use -- it actually got really annoying not being able to do:

foo := File{...}.Create(...)

foos := Entries{
    ...
    ...
}.Create(...)

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:65: func (e Entries) Removeds() Entries {
On 2014/03/27 09:39:24, rog wrote:
> AsRemoved ?
> (as it is, it sounds a bit like it's returning those
> entries that are instances of Removed)

SGTM, done.

[rog, 2014-04-03 11:34:19]

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:24: // a copy of the receiver.
On 2014/04/02 08:42:53, fwereade wrote:
> On 2014/03/27 09:39:24, rog wrote:
> > What's the point of returning a copy of the receiver? all your
implementations
> > are by-value anyway, so it's really a no-op.
> 
> Compactness in use -- it actually got really annoying not being able to do:
> 
> foo := File{...}.Create(...)
> 
> foos := Entries{
>     ...
>     ...
> }.Create(...)

personally i think that:

   foo := File{...}
   foo.Create(...)

reads just as naturally and is only one extra line, but fair enough. it's a pity
about the extra code to do it though (i bet you spend more lines of code here
than you save elsewhere)

https://codereview.appspot.com/80540043/diff/40001/testing/filetesting/filete...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/40001/testing/filetesting/filete...
testing/filetesting/filetesting.go:192: c.Assert(err, jc.Satisfies,
utils.IsNotExist)
?

https://codereview.appspot.com/80540043/diff/40001/utils/file_unix.go
File utils/file_unix.go (right):

https://codereview.appspot.com/80540043/diff/40001/utils/file_unix.go#newcode20
utils/file_unix.go:20: // reference a file that does not exist.
Perhaps add a comment that says why this is different from os.IsNotExist?

[rog, 2014-04-03 11:34:30]

LGTM, BTW.

[fwereade, 2014-04-03 16:39:56]

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/20001/testing/filetesting/filete...
testing/filetesting/filetesting.go:24: // a copy of the receiver.
On 2014/04/03 11:34:19, rog wrote:
> On 2014/04/02 08:42:53, fwereade wrote:
> > On 2014/03/27 09:39:24, rog wrote:
> > > What's the point of returning a copy of the receiver? all your
> implementations
> > > are by-value anyway, so it's really a no-op.
> > 
> > Compactness in use -- it actually got really annoying not being able to do:
> > 
> > foo := File{...}.Create(...)
> > 
> > foos := Entries{
> >     ...
> >     ...
> > }.Create(...)
> 
> personally i think that:
> 
>    foo := File{...}
>    foo.Create(...)
> 
> reads just as naturally and is only one extra line, but fair enough. it's a
pity
> about the extra code to do it though (i bet you spend more lines of code here
> than you save elsewhere)
> 

IMO it ends up looking icky in cases where you're setting up/checking a bunch of
entries, and sometimes you want to capture a var, and sometimes you don't. It's
not really about lines of code -- I think you'd win that bet :).

https://codereview.appspot.com/80540043/diff/40001/testing/filetesting/filete...
File testing/filetesting/filetesting.go (right):

https://codereview.appspot.com/80540043/diff/40001/testing/filetesting/filete...
testing/filetesting/filetesting.go:192: c.Assert(err, jc.Satisfies,
utils.IsNotExist)
On 2014/04/03 11:34:19, rog wrote:
> ?

Commented.

https://codereview.appspot.com/80540043/diff/40001/utils/file_unix.go
File utils/file_unix.go (right):

https://codereview.appspot.com/80540043/diff/40001/utils/file_unix.go#newcode20
utils/file_unix.go:20: // reference a file that does not exist.
On 2014/04/03 11:34:19, rog wrote:
> Perhaps add a comment that says why this is different from os.IsNotExist?

Done.