-
Notifications
You must be signed in to change notification settings - Fork 0
chore: remove placeholders #110
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
Summary of ChangesHello @b-long, 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 a significant cleanup 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 aims to clean up the codebase by removing several placeholder classes, methods, and parameters. While this is a good cleanup effort, the changes are incomplete and introduce several issues. Most critically, the SDKBuilder.build method in src/otdf_python/sdk_builder.py now makes an invalid call to the SDK constructor, which will cause a TypeError and break the build. This file must be updated as part of this PR. Additionally, there are issues with an outdated docstring in sdk.py and a failing test in test_sdk.py due to incomplete removal of related code. Please see the detailed comments for suggestions on how to fix these.
|
/gemini review |
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 performs a cleanup by removing several placeholder interfaces, classes, and methods, which aligns the SDK with the published roadmap. The changes are well-contained and correctly remove the unused code from the SDK, builder, and tests. I've found one minor issue where the docstring for SDK.__init__ was not fully updated after removing some parameters, which could lead to confusion.
|
/gemini review |
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 effectively removes a significant amount of placeholder code, including interfaces, classes, and methods, which cleans up the SDK and aligns it with the project's roadmap. The changes are consistent across the codebase, and the tests have been updated accordingly to reflect the removals.
While the cleanup is beneficial, there are some remaining pieces of related code that were not removed. For instance, in src/otdf_python/sdk_builder.py, the _create_auth_interceptor() method is still called within _create_services(), but its result is unused. Similarly, the auth_headers parameter in KAS.__init__ within src/otdf_python/sdk.py is also unused. Removing this leftover code in a subsequent pull request would complete the refactoring and further improve code maintainability.
* fix: use AuthHeaders, cleanup others * chore: run 'pre-commit autoupdate' * fix: place import at top (PEP 8 style)
|
/gemini review |
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 provides a significant cleanup by removing numerous placeholder interfaces, classes, and methods, which greatly improves the clarity and focus of the SDK. The changes are well-aligned with the project's roadmap. I appreciate the thoroughness of removing related code from tests and mocks, ensuring consistency. The refactoring to introduce the AuthHeaders class with its to_dict method is a nice improvement for handling authentication headers in a more structured way, and it's good to see it's backed by new unit tests.
Cleanup, to align well with the Roadmap published in #111 .
The following placeholder interfaces and classes were removed:
Service Client Interfaces (Removed)
AttributesServiceClientInterfaceNamespaceServiceClientInterfaceSubjectMappingServiceClientInterfaceResourceMappingServiceClientInterfaceAuthorizationServiceClientInterfaceKeyAccessServerRegistryServiceClientInterfaceInfrastructure Placeholders (Removed)
ProtocolClientclassInterceptorclassTrustManagerclassSDK Constructor Parameters (Removed)
trust_managerparameterauth_interceptorparameterplatform_services_clientparameterServices Interface Methods (Removed)
attributes()methodnamespaces()methodsubject_mappings()methodresource_mappings()methodauthorization()methodkas_registry()methodSDK Getter Methods (Removed)
get_trust_manager()get_auth_interceptor()get_platform_services_client()