Code review comment for lp:~niemeyer/pyjuju/go-charm-bits

Revision history for this message
Roger Peppe (rogpeppe) wrote :

general +1

[1]
charm/bundle.go
 for _, header := range zipr.File {

Seems to me like the body of this loop would be better
in a separate function. Then you could just defer
zf.Close and things would be generally cleaner.

[2]
charm/dir.go
 parts = append([]string{dir.Path}, parts...)

appending to parts directly is bad behaviour because
it could interfere with any callers that call join
with a slice. i know you don't do that, but it sets
a dubious example.

[3]
charm/dir.go
 hidden := len(relpath) > 1 && relpath[0] == '.'

I hope this behaviour is well documented. I've provided
config files as part of a charm before, and it's only
luck that they didn't start with a ".".
If the object is to exclude source-code control files
(e.g. .hg and .bzr) perhaps it would be better to
name the exclusions specifically, rather than excluding
all files starting with ".".

[4]
charm/reltest.go
  got, err := filepath_Rel(test.root, test.path)

i thought Rel had now been accepted.

« Back to merge proposal