-
Notifications
You must be signed in to change notification settings - Fork 0
[PD1-581] Display CloudFront ID in csv_import error messages #7
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
Conversation
PD1-581 Make tag directories index multi-tenant
This solves a problem where new data cannot be loaded into new tenant because of a (false) tag name ID collision. We only want unique IDs within a tenant, not across all tenants. |
Summary of ChangesHello @joshuanapoli, 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! This pull request focuses on improving the robustness and debuggability of the Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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. Footnotes
|
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.
Code Review
This pull request improves error handling in the csv_import.py script by displaying the CloudFront ID from the x-amz-cf-id header on HTTP errors, which will be very helpful for debugging. The addition of types-requests is also a good step towards better type safety.
My review includes a few suggestions:
- In
csv_import.py, I've pointed out an unsorted import and a hardcoded string that could be defined as a constant for better maintainability. - In
scripts/lint.sh, I've identified a potential issue with howpytestis being called, which could lead to inefficiency by always running the full test suite.
Additionally, with the inclusion of types-requests, you should now be able to remove the # type: ignore[import-untyped] comment next to the import requests statement in src/cvec/cvec.py.
Overall, these are useful changes.
| poetry run mypy --strict "$@" | ||
|
|
||
| echo -e "\npoetry run pytest" | ||
| poetry run pytest |
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.
The lint.sh script is now also running tests, which is a significant change in its responsibility. More importantly, pytest is run without arguments, which means it will execute the entire test suite regardless of the file paths passed to lint.sh. This can be very inefficient, especially when linting a single file. If the intention is to run tests only for the specified files, you should pass the arguments to pytest.
| poetry run pytest | |
| poetry run pytest "$@" |
| print(f"Error: {e}") | ||
| # Display CloudFront ID if available | ||
| if e.response is not None: | ||
| cf_id = e.response.headers.get("x-amz-cf-id") |
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.
Added error handling to display the x-amz-cf-id response header when HTTP requests fail, making it easier to debug server-side issues. Also added types-requests to dev dependencies for better type checking. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
CharlesKleeven
left a comment
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.
Thanks!
Added error handling to display the x-amz-cf-id response header when HTTP requests fail, making it easier to debug server-side issues. Also added types-requests to dev dependencies for better type checking.
🤖 Generated with Claude Code
Testing
Author
I used this change to debug a problem with setting up a new tenant.