Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions bagit.py
Original file line number Diff line number Diff line change
Expand Up @@ -766,9 +766,9 @@ def _make_manifest(manifest_file, data_dir, processes, algorithm='md5'):
def _make_tagmanifest_file(alg, bag_dir):
tagmanifest_file = join(bag_dir, "tagmanifest-%s.txt" % alg)
LOGGER.info("writing %s", tagmanifest_file)
files = [f for f in listdir(bag_dir) if isfile(join(bag_dir, f))]

checksums = []
for f in files:
for f in _find_tag_files(bag_dir):
if re.match(r'^tagmanifest-.+\.txt$', f):
continue
with open(join(bag_dir, f), 'rb') as fh:
Expand All @@ -784,6 +784,15 @@ def _make_tagmanifest_file(alg, bag_dir):
for digest, filename in checksums:
tagmanifest.write('%s %s\n' % (digest, filename))

def _find_tag_files(bag_dir):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

for dirName, subdirList, fileList in os.walk(bag_dir):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if not re.match(r'.*data$', dirName):
for filename in fileList:
if re.match(r'^tagmanifest-.+\.txt$', filename):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than using a file regex, this could be faster and potentially easier to read as if filename.startswith('tagmanifest-')

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

continue
#remove everything up to the bag_dir directory
p=join(dirName, filename)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be p = following PEP-8.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

yield p[len(bag_dir)+1:]
Copy link
Member

@acdha acdha Sep 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool, didn't know that this existed. Done


def _walk(data_dir):
for dirpath, dirnames, filenames in os.walk(data_dir):
Expand Down
31 changes: 31 additions & 0 deletions test.py
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,37 @@ def test_validate_optional_tagfile(self):
os.remove(j(tagdir, "tagfile"))
bag = bagit.Bag(self.tmpdir)
self.assertRaises(bagit.BagValidationError, self.validate, bag)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds trailing whitespace, which makes diffs noisier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

def test_validate_optional_tagfile_in_directory(self):
bag = bagit.make_bag(self.tmpdir)
tagdir = tempfile.mkdtemp(dir=self.tmpdir)

if not os.path.exists(j(tagdir, "tagfolder")):
os.makedirs(j(tagdir, "tagfolder"))

with open(j(tagdir, "tagfolder", "tagfile"), "w") as tagfile:
tagfile.write("test")
relpath = j(tagdir, "tagfolder", "tagfile").replace(self.tmpdir + os.sep, "")
relpath.replace("\\", "/")
with open(j(self.tmpdir, "tagmanifest-md5.txt"), "w") as tagman:
# Incorrect checksum.
tagman.write("8e2af7a0143c7b8f4de0b3fc90f27354 " + relpath + "\n")
bag = bagit.Bag(self.tmpdir)
self.assertRaises(bagit.BagValidationError, self.validate, bag)

hasher = hashlib.new("md5")
with open(j(tagdir, "tagfolder", "tagfile"), "r") as tf:
contents = tf.read().encode('utf-8')
hasher.update(contents)
with open(j(self.tmpdir, "tagmanifest-md5.txt"), "w") as tagman:
tagman.write(hasher.hexdigest() + " " + relpath + "\n")
bag = bagit.Bag(self.tmpdir)
self.assertTrue(self.validate(bag))

# Missing tagfile.
os.remove(j(tagdir, "tagfolder", "tagfile"))
bag = bagit.Bag(self.tmpdir)
self.assertRaises(bagit.BagValidationError, self.validate, bag)

def test_sha1_tagfile(self):
info = {'Bagging-Date': '1970-01-01', 'Contact-Email': '[email protected]'}
Expand Down