-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: address issues in code review #72
Conversation
PR Reviewer Guide 🔍(Review updated until commit 5220405)Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Latest suggestions up to 5220405
Previous suggestionsSuggestions up to commit 5220405
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -77,6 +77,8 @@ markdown = ap.batches.retrieve(request_id) | |||
``` | |||
|
|||
> ⚠️ **Note:** Batch extraction is currently in beta testing. Processing time may take up to 12 hours to complete. | |||
> | |||
> ⚠️ **Important:** API keys generated from cambioml.com do not automatically have batch processing permissions. Please contact [email protected] to request batch processing access for your API key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
any_parser/batch_parser.py:68
- The error message raised by FileNotFoundError could be more informative. Consider updating it to: 'The file path '{file_path}' does not exist. Please provide a valid file path.'
raise FileNotFoundError(f"The file path '{file_path}' does not exist.")
reopen to trigger workflow |
Persistent review updated to latest commit 5220405 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
User description
Description
This PR includes several improvements to the batch processing functionality and error handling:
Type of Change
How Has This Been Tested?
Screenshots (if applicable)
Checklist
Additional Notes
PR Type
Bug fix, Documentation, Enhancement
Description
PUBLIC_BATCH_BASE_URL
to use the production domain for batch processing.AnyParser
class by adding abatch_url
parameter for better flexibility.BatchParser
by adding a validation check for file existence.batch_parser.py
for better clarity and maintainability.Changes walkthrough 📝
any_parser.py
Update batch URL and enhance AnyParser initialization.
any_parser/any_parser.py
PUBLIC_BATCH_BASE_URL
to use the production domain.batch_url
parameter toAnyParser
class initialization.BatchParser
initialization to use thebatch_url
parameter.batch_parser.py
Improve batch file handling and response model documentation.
any_parser/batch_parser.py
README.md
Update README with batch processing permissions note.
README.md