cmd: refactor FileVar to retrieve contents

cmd: refactor FileVar to retrieve contents

Stop being so clever, FileVar.Path is non nil if a config file was passed, Read(*cmd.Context) returns the contents of the file with respect to the Context.

https://code.launchpad.net/~dave-cheney/juju-core/104-cmd-juju-config-file-open-at-runtime/+merge/124933

(do not edit description out of merge proposal)

[fwereade, 2012-09-18 14:10:35]

Just one comment apart form the on mentioned live.

https://codereview.appspot.com/6527045/diff/1/cmd/filevar.go
File cmd/filevar.go (left):

https://codereview.appspot.com/6527045/diff/1/cmd/filevar.go#oldcode16
cmd/filevar.go:16: // Set opens the file.
Doesn't open the file any more.

https://codereview.appspot.com/6527045/diff/1/cmd/filevar.go
File cmd/filevar.go (right):

https://codereview.appspot.com/6527045/diff/1/cmd/filevar.go#newcode12
cmd/filevar.go:12: Path *string
Why a pointer?

[dfc, 2012-09-18 14:12:59]

Please take a look.

[niemeyer, 2012-09-18 14:21:06]

LGTM

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go
File cmd/filevar.go (right):

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go#newcode21
cmd/filevar.go:21: func (f *FileVar) Open(ctx *Context) (io.ReadCloser, error) {
Is there a reason for this to not be a *os.File?

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar_test.go
File cmd/filevar_test.go (right):

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar_test.go#newcode40
cmd/filevar_test.go:40: defer r.Close()
s/defer//

[dfc, 2012-09-18 14:29:44]

Thank you for your review.

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go
File cmd/filevar.go (right):

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go#newcode21
cmd/filevar.go:21: func (f *FileVar) Open(ctx *Context) (io.ReadCloser, error) {
On 2012/09/18 14:21:06, niemeyer wrote:
> Is there a reason for this to not be a *os.File?

Maybe it should just be Contexts(ctx) ([]byte, error) as that is what we're
doing to do with it.

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar_test.go
File cmd/filevar_test.go (right):

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar_test.go#newcode40
cmd/filevar_test.go:40: defer r.Close()
On 2012/09/18 14:21:06, niemeyer wrote:
> s/defer//

Done.

[niemeyer, 2012-09-18 14:47:48]

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go
File cmd/filevar.go (right):

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go#newcode21
cmd/filevar.go:21: func (f *FileVar) Open(ctx *Context) (io.ReadCloser, error) {
On 2012/09/18 14:29:44, dfc wrote:
> On 2012/09/18 14:21:06, niemeyer wrote:
> > Is there a reason for this to not be a *os.File?
> 
> Maybe it should just be Contexts(ctx) ([]byte, error) as that is what we're
> doing to do with it.

Contexts? You mean something like Read(ctx)? That sounds fine as well.

[dfc, 2012-09-18 14:56:54]

*** Submitted:

cmd: refactor FileVar to retrieve contents

Stop being so clever, FileVar.Path is non nil if a config file was passed,
Read(*cmd.Context) returns the contents of the file with respect to the Context.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6527045

[fwereade, 2012-09-18 14:57:12]

LGTM modulo comment below

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go
File cmd/filevar.go (left):

https://codereview.appspot.com/6527045/diff/3001/cmd/filevar.go#oldcode16
cmd/filevar.go:16: // Set opens the file.
Still inaccurate.