search

NYPL / ami-tools

MIT License
14 stars 6 forks source link

Update validate_ami_bags.py #93

Closed bturkus closed 4 months ago

bturkus commented 4 months ago

better handling of multiple directories of bags summary reporting per dir at the end of the script

bturkus commented 4 months ago

@cgmcnamara I tested this pretty thoroughly, but please review when you have time. Instead of grouping mult dirs of bags together, the script will now process each dir individually. Reporting for all dirs is saved for the end. Tested w/ --metadata flag, and -b on a single bag

nkrabben commented 4 months ago

Butting in to say that this looks clean, and adds functionality.

We're getting ready to start reviewing the concept of AMI bag validation. No reason to stop this PR from being merged, but if you think of anything while reviewing, keep it in mind and ready to share please.

cgmcnamara commented 4 months ago

Thanks Ben this seems good! I suggested a few changes related to reporting. Let me know if I did this right on GitHub...

And thanks for the heads up, Nick! I did notice an issue where "warning" bags reporting could be improved but I don't totally understand it yet.

bturkus commented 4 months ago

Was there a particular reason to change this line:

"Checking {} folder(s) in directory: {}".format(len(bags), directory_path))"

I thought it would be nice to have number of bags per project directory in a print statement; is thie happening in some other fashion after your changes?

bturkus commented 4 months ago

ok sounds good. I think you still need to approve of the original PR for me to merge

cgmcnamara commented 4 months ago

Was there a particular reason to change this line:

"Checking {} folder(s) in directory: {}".format(len(bags), directory_path))"

I thought it would be nice to have number of bags per project directory in a print statement; is thie happening in some other fashion after your changes?

My change is not crucial! Happy to leave it as you had it. Here are two examples of how it prints...

... the way you had it:

Now checking this directory: /Volumes/pami/x_CaseyTest/MDR0001121_copy
root: 2024-04-29 13:43:50,592 - INFO - Checking 11 folder(s) in directory: /Volumes/pami/x_CaseyTest/MDR0001121_copy

... the way I changed (reverted) it: 

Now checking this directory: /Volumes/pami/x_CaseyTest/MDR0001121_copy
root: 2024-04-29 13:43:50,592 - INFO - Checking 11 folder(s)
bturkus commented 4 months ago

ok whatever works, can you approve?

cgmcnamara commented 4 months ago

yes! thanks for your patience while I figure out GitHub