Conversation
acdha
left a comment
acdha
left a comment
There was a problem hiding this comment.
Generally looks good to me but I would change the filename processing for clarity
bagit.py
Outdated
There was a problem hiding this comment.
This should be p = following PEP-8.
| for digest, filename in checksums: | ||
| tagmanifest.write('%s %s\n' % (digest, filename)) | ||
|
|
||
| def _find_tag_files(bag_dir): |
There was a problem hiding this comment.
I think the Python community has increasingly discouraged the convention of using _ prefixes like this, but there's precedent in the other functions in this file. I'd be inclined to remove it.
bagit.py
Outdated
There was a problem hiding this comment.
PEP-8 encourages dir_name, etc. I generally prefer to use plural names like subdirectories, filenames, rather than a list suffix but am ±0 on that.
bagit.py
Outdated
There was a problem hiding this comment.
Rather than using a file regex, this could be faster and potentially easier to read as if filename.startswith('tagmanifest-')
bagit.py
Outdated
There was a problem hiding this comment.
Stylistically, this should have spaces around operators (len(bag_dir) + 1) following PEP-8.
Logically, if I'm understanding it correctly this could also be written as yield os.path.relpath(p, start=bag_dir)
There was a problem hiding this comment.
cool, didn't know that this existed. Done
test.py
Outdated
There was a problem hiding this comment.
This adds trailing whitespace, which makes diffs noisier
2 participants
Shoudl fix issue #68