Conversation
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. |
doctrino
force-pushed
the
groups-crud
branch
from
2198152 to
d67b2f7
Compare
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant refactoring that replaces the legacy SDK classes for Groups with new Pydantic-based models. This is a great improvement for type safety and consistency with the newer parts of the toolkit. The changes are extensive, touching the API client, CRUD logic, command-line tools, and tests.
I've found two critical issues related to the new GroupsAPI.list implementation that break pagination, which will affect any operation that needs to list all groups. Please see my detailed comments.
Other than these critical issues, the refactoring looks solid and well-executed. The use of Pydantic models and validators to handle API inconsistencies is a good practice.
doctrino
force-pushed
the
groups-crud
branch
from
347c854 to
ee9fb13
Compare
| # The dataproduct API is not yet supported in CDF. | ||
| DataProductsAPI, |
There was a problem hiding this comment.
@ronpal Note that I forgot to comment on this in your PR, but without it the smoke tests will fail tomorrow morning
| def convert_scope_format(cls, value: Any) -> Any: | ||
| """Preserve original scope for unknown ACLs.""" | ||
| if not isinstance(value, dict): | ||
| return value | ||
| return value |
There was a problem hiding this comment.
Nothing, both that and the above. It slipped passed me in my review of the agent.
| List of GroupResponse objects. | ||
| Args: | ||
| all_groups: Whether to return all groups (requires admin permissions). | ||
| . |
| try: | ||
| listed = api.list(limit=10) | ||
| except TypeError: | ||
| listed = api.list() # Some APIs do not support limit parameter |
There was a problem hiding this comment.
Too broad? Can it mask other errors?
There was a problem hiding this comment.
I don't think so, since we do the exact same call without the limit parameter right after. If there is another exception, it will raised then.
I can do something like
try:
listed = api.list(limit=10)
except TypeError as e:
if "unexpected keyword argument 'limit'" in str(e):
listed = api.list() # Some APIs do not support limit parameter
else:
raise e
but I think it is a bit messy.
Alternative is to write a custom test for GroupsAPI.
What do you think?
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2525 +/- ##
==========================================
- Coverage 85.84% 85.83% -0.01%
==========================================
Files 414 414
Lines 34386 34397 +11
==========================================
+ Hits 29518 29526 +8
- Misses 4868 4871 +3
🚀 New features to boost your workflow:
|
2 participants
Description
Replace the PySDK with pydantic classes for Gropus.
Bump
Changelog
Improved