Add structured output export to CSV and JSON #24
Conversation
bradleyrule
left a comment
bradleyrule
left a comment
There was a problem hiding this comment.
Looks pretty good!
Overarching change to make is converting function description comments to docstrings.
A few issues in full_pipeline.py that will need to be addressed before merging.
| from scripts.env_loader import load_env | ||
| load_env() | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Is there a reason that this is just set to pass on an exception? I assume we'd want some form of error logging here.
| print("Please install: pip install google-auth google-api-python-client") | ||
| return False | ||
|
|
||
| root_id = os.environ.get("GOOGLE_DRIVE_FOLDER_ID") |
There was a problem hiding this comment.
This change to "GOOGLE_DRIVE_FOLDER_ID" causes the script to be unable to find the drive folder since the environment variable is defined as "GOOGLE_DRIVE_ROOT_FOLDER_ID" in the .env file. This will need to be reverted to use the old variable naming scheme (unless we have a new .env that uses this new naming scheme that I am unaware of)
| - Custom path: python full_pipeline.py --local C:\\path\\to\\data | ||
|
|
||
| Behavior: | ||
| - API mode: Streams every PDF (no local PDF persistence) and writes extracted text to data/processed-text. |
There was a problem hiding this comment.
Might want to leave in the distinction between api mode and local mode in the behavior section.
Might also help to change line 4 to improve clarity since PDFs are streamed and not downloaded.
| def process_api_mode(): | ||
| """Download PDFs from Google Drive and process them.""" | ||
| root_id = os.environ.get("GOOGLE_DRIVE_ROOT_FOLDER_ID") | ||
| #Download PDFs from Google Drive and process them |
There was a problem hiding this comment.
Our style guide says to use docstrings instead of comments for function descriptions, so it would be best to revert this to """Download PDFs from Google Drive and process them."""
Other places where this change may need to be made:
- Line 118
- Line 130
- Line 173
| if not GOOGLE_DRIVE_AVAILABLE: | ||
| print("ERROR: Google Drive modules not available.") | ||
| print("Please install: pip install google-auth google-api-python-client") | ||
| return False |
There was a problem hiding this comment.
Would be better to raise an error instead of returning False. This keeps with current code conventions and provides more detailed error handling and logging.
| success = process_api_mode() | ||
| if not success: | ||
| sys.exit(1) |
There was a problem hiding this comment.
Related to comment on line 77-80: Probably better to catch specific error you are looking for with a try / except block or allow error to go uncaught here so it can be caught and handled higher up the call stack (or terminated if not caught).
| # Create mutually exclusive group for --api and --local | ||
| group = parser.add_mutually_exclusive_group(required=True) | ||
| group.add_argument( |
There was a problem hiding this comment.
Is there a reason we changed this? It seems like a good feature to prevent the use of both arguments.
There was a problem hiding this comment.
Probably best to change comments describing function purpose to docstrings
There was a problem hiding this comment.
Best to change function descriptions to docstrings
No description provided.