Improve file discovery tests and inject PDF converter - #1
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @SuperMuel, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello team, gemini-code-assist here to provide a summary of this pull request. This PR, authored by SuperMuel, focuses on improving the robustness of file discovery and enhancing the testability of the PDF processing logic. The main changes involve making the PDF file discovery case-insensitive, injecting dependencies into the process_and_save_pdf function to facilitate testing, and adding a comprehensive suite of unit tests covering file discovery, output path planning, and the newly refactored processing function.
Highlights
- Case-Insensitive PDF Discovery: The
find_pdf_filesfunction has been updated to correctly identify PDF files regardless of the case of the.pdfextension (e.g.,.PDF,.pdf). - Dependency Injection: The
process_and_save_pdffunction now accepts optionalprocess_func,extract_func, andsave_funcarguments, allowing external code (like tests) to inject custom implementations of these steps. - Extensive Unit Tests: A new test file (
tests/test_file_discovery.py) has been added containing multiple tests forfind_pdf_files,determine_output_directory,create_conversion_plan, andprocess_and_save_pdf(using injected mocks).
Changelog
Click here to see the changelog
- src/mistral_ocr/init.py
- Modified
find_pdf_filesto userglob('*')and check the lowercased suffix (path.suffix.lower() == '.pdf') to support case-insensitive PDF extension matching.
- Modified
- src/mistral_ocr/ocr_utils.py
- Added
Callableimport fromtyping. - Updated the signature of
process_and_save_pdfto include keyword-only argumentsprocess_func,extract_func, andsave_funcwith default values pointing to the original functions. - Modified the implementation of
process_and_save_pdfto call the injected functions (process_func,extract_func,save_func) instead of the hardcoded ones.
- Added
- tests/test_file_discovery.py
- Added a new test file.
- Added tests for
find_pdf_filescovering single files, directories with subdirectories, case-insensitivity, and missing paths. - Added tests for
determine_output_directorycovering directory and file inputs. - Added tests for
create_conversion_plancovering output path generation and skipping existing files. - Added a test for
process_and_save_pdfdemonstrating the use of injected dummy functions to verify its control flow.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Tests now check the case,
Dependencies find their place,
Code is easier to trace.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces improvements to file discovery, dependency injection for PDF processing, and extensive unit tests. The changes enhance the flexibility and testability of the OCR processing pipeline. Overall, the changes look good, but there are a few areas that could be improved.
Summary of Findings
- File Discovery: The case-insensitive PDF discovery is a good improvement. Consider adding a comment explaining the use of
rgloband potentially exploringPath.globwith case-insensitive patterns for performance. - Dependency Injection: The dependency injection in
process_and_save_pdfenhances testability. Adding type hints and more descriptive parameter names would further improve readability.
Merge Readiness
The pull request is in good shape and introduces valuable improvements. Addressing the suggestions regarding comments and type hints would further enhance the code quality. I am unable to directly approve the pull request, and users should have others review and approve this code before merging.
| return [ | ||
| path | ||
| for path in input_path.rglob("*") | ||
| if path.suffix.lower() == ".pdf" | ||
| ] |
There was a problem hiding this comment.
This is a good change to make the PDF discovery case-insensitive. However, consider adding a comment explaining why rglob is used instead of glob for future maintainers. Also, consider using Path.glob with a case-insensitive glob pattern if supported by the underlying OS, as it might be more performant than filtering with a list comprehension.
| return [ | |
| path | |
| for path in input_path.rglob("*") | |
| if path.suffix.lower() == ".pdf" | |
| ] | |
| # Use rglob to find all files recursively and filter for case-insensitive PDF suffix. | |
| return [ | |
| path | |
| for path in input_path.rglob("*") | |
| if path.suffix.lower() == ".pdf" | |
| ] |
| def process_and_save_pdf( | ||
| client: Mistral, input_path: Path, output_dir: Path, force: bool = False | ||
| client: Mistral, | ||
| input_path: Path, | ||
| output_dir: Path, | ||
| force: bool = False, | ||
| *, | ||
| process_func: Callable[[Mistral, Path], OCRResponse] = process_pdf_file, | ||
| extract_func: Callable[[OCRResponse], str] = extract_markdown_from_response, | ||
| save_func: Callable[[str, Path], None] = save_markdown_to_file, | ||
| ) -> tuple[bool, str, ProcessedDocument | None]: |
There was a problem hiding this comment.
The injection of dependencies into process_and_save_pdf is a good practice for testability and flexibility. However, consider adding type hints for the process_func, extract_func, and save_func parameters to improve code readability and maintainability. Also, consider renaming the parameters to be more descriptive, e.g. pdf_processor, response_extractor, markdown_saver.
| def process_and_save_pdf( | |
| client: Mistral, input_path: Path, output_dir: Path, force: bool = False | |
| client: Mistral, | |
| input_path: Path, | |
| output_dir: Path, | |
| force: bool = False, | |
| *, | |
| process_func: Callable[[Mistral, Path], OCRResponse] = process_pdf_file, | |
| extract_func: Callable[[OCRResponse], str] = extract_markdown_from_response, | |
| save_func: Callable[[str, Path], None] = save_markdown_to_file, | |
| ) -> tuple[bool, str, ProcessedDocument | None]: | |
| def process_and_save_pdf( | |
| client: Mistral, | |
| input_path: Path, | |
| output_dir: Path, | |
| force: bool = False, | |
| *, | |
| pdf_processor: Callable[[Mistral, Path], OCRResponse] = process_pdf_file, | |
| response_extractor: Callable[[OCRResponse], str] = extract_markdown_from_response, | |
| markdown_saver: Callable[[str, Path], None] = save_markdown_to_file, | |
| ) -> tuple[bool, str, ProcessedDocument | None]: |
1 participant
Summary
process_and_save_pdfTesting
uv run python -m pytesthttps://chatgpt.com/codex/tasks/task_e_68416c86dd188328909c7f8f27a7d1d2