Logging rollcall#67
Conversation
egrace479
left a comment
egrace479
left a comment
There was a problem hiding this comment.
Comments and suggestions specifically looking at these changes.
It seems some informative comments were also removed. Looking at the full diff, all the check_existing_images docstrings have been removed. This function should have been essentially copied over with only the logging updates.
| try: | ||
| existing_files = gather_file_paths(img_dir) | ||
| except EmptyInputDirectoryError: | ||
| logger.warning(f"Image directory '{img_dir}' is empty.") |
There was a problem hiding this comment.
This wouldn't be a warning, just info. It falls into almost the same category as above where the directory doesn't exist. These are testing the target image directory.
|
|
||
| num_existing = len(existing_files) | ||
| logger.info( | ||
| f"{num_existing} files already exist in {img_dir}. " |
There was a problem hiding this comment.
This isn't actually about the number of files in the target directory. It's specifically the number of files in the target directory that match those listed in the provided CSV (so this wording needs to change to match the meaning from the print message).
| def _preview_or_save(self, label, df): | ||
| """Print up to 5 rows or save to CSV if larger.""" | ||
| if len(df) <= 5: | ||
| logger.warning(f"{label} detected ({len(df)} rows). Previewing.") | ||
| print(f"\n❗ {label.replace('_', ' ').title()} detected (showing all):") | ||
| print(df) | ||
| else: | ||
| csv_base = os.path.splitext(self.csv_path)[0] | ||
| save_path = f"{csv_base}_{label}.csv" | ||
| df.to_csv(save_path, index=False) | ||
|
|
||
| logger.warning(f"{len(df)} {label} detected. Saved to {save_path}") | ||
|
|
||
| print( | ||
| f"\n❗ {len(df)} {label.replace('_', ' ')} detected.\n" | ||
| f"Full list saved to:\n {save_path}\n" | ||
| ) |
There was a problem hiding this comment.
Would these warnings have meaning outside the log notes you set for the missing values. Looking at these functions, it's unclear the application and if there would be sufficient context to understand.
| return | ||
|
|
||
| # If duplicates exist, preview or save | ||
| logger.warning(f"Duplicate checksums detected ({len(dupes)} rows).") |
There was a problem hiding this comment.
I think this would be better as a sentence "...detected in {len(dupes)} rows."
| logger.warning("Ignoring duplicates due to --ignore-duplicates flag.") | ||
| print( | ||
| f"\n⚠ Duplicate checksums detected ({len(dupes)} rows), " | ||
| f"\n Duplicate checksums detected ({len(dupes)} rows), " |
| @patch("builtins.print") | ||
| def test_missing_filenames_returns_correct_subset(self, mock_print): | ||
| """Should return only rows with missing filenames and valid URLs.""" | ||
| def test_handle_missing_filenames_some_missing(self): |
There was a problem hiding this comment.
This is all missing. There should be tests for none missing (with more than one entry), some missing (combination), and all missing (also with more than one entry).
The same note applies to all other cases below (i.e., all images already exist, some, none).
Might also wish to check for appropriate logs, certainly in the cases where there are potential issues.
2 participants
Integrate logging functionality into the RollCall module.