Code review comment for lp:~themue/juju-core/033-sync-tools-source

Please take a look.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go
File cmd/juju/synctools.go (right):

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode50
cmd/juju/synctools.go:50: to avoid having to access data outside of the
local cloud.
On 2013/07/11 20:40:40, dimitern wrote:
> please add an example of the two kinds of --source options: file path
and url.

Done, but only added a hint to the option to specify a source directory.
The idea of alternatively pass a HTTP location has been postponed.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode61
cmd/juju/synctools.go:61: f.StringVar(&c.source, "source", "default",
"chose a location on the file system or via http as source")
On 2013/07/11 20:40:40, dimitern wrote:
> s/"default"/""/ (see below)

Done, also changed wrong help text.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode186
cmd/juju/synctools.go:186: if sourceFlagValue == "" || sourceFlagValue
== "default" {
On 2013/07/11 20:40:40, dimitern wrote:
> "default" seems icky - how's this different from "" ? why do we need
it?

Idea has been to give better chance to make it explicit. But you're
right, it's not needed, so removed it.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode206
cmd/juju/synctools.go:206: return nil, fmt.Errorf("specified source path
is no directory: %s", p)
On 2013/07/11 20:40:40, dimitern wrote:
> s/no/not a/, also if you're saying "specified", please pass `path`
instead of
> `p`, since that what was passed before Clean (less confusing for the
user)

Done.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode226
cmd/juju/synctools.go:226: pathlen := len(f.path) + 1
On 2013/07/11 20:40:40, dimitern wrote:
> maybe len(f.path) + len("/") is less ambiguous.

Simply added a comment. ;)

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode243
cmd/juju/synctools.go:243: sort.Strings(list)
On 2013/07/11 20:40:40, dimitern wrote:
> why are you sorting it?

See interface specification: "... in alphabetical order ...".

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools.go#newcode249
cmd/juju/synctools.go:249: return path.Join(f.path, name), nil
On 2013/07/11 20:40:40, dimitern wrote:
> in this case it should be filepath.Join, I think

Had it before, but see discussion with John.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go
File cmd/juju/synctools_test.go (right):

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode53
cmd/juju/synctools_test.go:53: // Create a local tool directory.
On 2013/07/11 20:40:40, dimitern wrote:
> s/tool/tools/

Done.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode106
cmd/juju/synctools_test.go:106: c.Assert(ctx, NotNil)
On 2013/07/11 20:40:40, dimitern wrote:
> shouldn't you be testing for specific contents of ctx instead?

See tests below, simply done it the same way. Don't know the original
intention, but this way nobody should ask why the test differs from
those below.

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode213
cmd/juju/synctools_test.go:213: dir, _ := filepath.Split(path)
On 2013/07/11 20:40:40, dimitern wrote:
> isn't dir the same as storagePath after this, use that instead? why
the split?
> why not Base or Dir?

It's not the same. storagePath may be /foo, the tool name
tools/bar-1.2.3.tar.gz. So the dir will be /foo/tools. But using Dir()
is good, simply used Split() too often. ;)

https://codereview.appspot.com/11103043/diff/6001/cmd/juju/synctools_test.go#newcode216
cmd/juju/synctools_test.go:216: file, err := os.Create(path)
On 2013/07/11 20:40:40, dimitern wrote:
> why not ioutil.WriteFile instead of most of the block below?

Aaargh, good catch. I had in mind that there's a simple helper, but
somehow I missed to use it. Thanks.

https://codereview.appspot.com/11103043/