feat(client): expose client cookie jar access#477
Conversation
Summary of ChangesHello @serozhenka, 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 enhances the client's cookie management capabilities by exposing the underlying cookie jar directly through a new 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.
Code Review
This pull request introduces a cookie_jar property to both the async and blocking Client classes, allowing users to access and manipulate the cookie jar used by the client. This is a great feature addition.
My review focuses on a few areas:
- A critical logic bug in the Rust implementation that incorrectly handles client configuration when both
cookie_providerandcookie_store=Trueare specified. I've provided a suggested fix. - A missing type hint and incomplete docstring in the Python stub for the blocking client.
- A suggestion to enhance the new tests to cover the edge case that would have revealed the aforementioned bug.
The changes are otherwise well-implemented, and the new tests provide good coverage for the main use cases. Addressing these points will make the feature more robust and maintainable.
|
Overall, it's not bad. Some modifications have been made. The |
|
Optional cookie jar sounds like a code smell. |
|
Claiming that an optional cookie jar is a “code smell” overlooks the benefits of clear and flexible design. Marking it as Optional explicitly indicates it may not always be available—a better signal than relying on AttributeError, which can conflate missing cookies with other programming errors. While handling None might add a minor overhead, it ensures safer, more predictable code and aligns well with modern type-checking tools like MyPy, ultimately reducing debugging efforts in edge cases. |
Resolves #476.
P.S. I know barely anything about Rust, so forgive if something is not up to with how it should be properly done. Feel free to contribute directly instead of asking me to make some changes, especially if it's Rust related.