chore: add @override decorators to LoggingPlugin callback methods#4572
chore: add @override decorators to LoggingPlugin callback methods#4572cchinchilla-dev wants to merge 7 commits intogoogle:mainfrom
Conversation
Summary of ChangesHello @cchinchilla-dev, 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 static analysis capabilities of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 adds @override decorators to the callback methods in LoggingPlugin that override methods from BasePlugin. This is a great improvement for static analysis and code maintainability, ensuring that any future changes to the base class methods are caught by tools like mypy. The changes are correct and well-implemented. No issues found.
|
Hi @cchinchilla-dev , Thank you for your contribution! We appreciate you taking the time to submit this pull request. Can you fix the formatting errors before we can proceed with the review. You can use autoformat.sh. |
|
Hi @ryanaiagent, thanks for the heads up! I’ve applied autoformat.sh and pushed the changes to the affected file. Running the script also introduced modifications in |
|
Hi @cchinchilla-dev ,you can ignore those files. |
|
Hi @Jacksunwei , can you please review this. LGTM. |
|
Hi @ryanaiagent! Just checking in to see if there’s anything else needed from my side. |
Merge #4572 ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Closes: #4496 **2. Or, if no issue exists, describe the change:** **Problem:** `LoggingPlugin` overrides 12 callback methods from `BasePlugin` but none use the `@override` decorator. Every other plugin in the package (`DebugLoggingPlugin`, `ReplayPlugin`, `RecordingsPlugin`, `EnsureRetryOptionsPlugin`, `_RequestIntercepterPlugin`) already follows this practice. With `mypy --strict` enabled in `pyproject.toml`, missing `@override` means renamed or removed base-class methods would be silently missed in `LoggingPlugin` while being caught everywhere else. **Solution:** Import `override` from `typing_extensions` and decorate all 12 overridden callbacks. Purely additive: one import line and 12 decorators. No behavioral, API, or runtime change. ### Testing Plan **Unit Tests:** - [ ] I have added or updated unit tests for my change. - [x] All unit tests pass locally. No new tests are required — `@override` is a static-analysis-only decorator with no runtime effect. Ran `mypy` on the file before and after the change: same preexisting warnings, no new errors introduced. CI will validate via the existing test suite and linting checks. **Manual End-to-End (E2E) Tests:** Not applicable. This change adds only decorators with no runtime behavior. Verified by comparing `mypy` output before and after — no new errors. ### Checklist - [x] I have read the [CONTRIBUTING.md](https://github.com/google/adk-python/blob/main/CONTRIBUTING.md) document. - [x] I have performed a self-review of my own code. - [x] I have commented my code, particularly in hard-to-understand areas. - [ ] I have added tests that prove my fix is effective or that my feature works. - [x] New and existing unit tests pass locally with my changes. - [ ] I have manually tested my changes end-to-end. - [ ] Any dependent changes have been merged and published in downstream modules. ### Additional context I am the author of the original issue (#4496). A previous PR (#4544) was opened but is pending clarification, so I'm submitting this complete PR as the original issue author. COPYBARA_INTEGRATE_REVIEW=#4572 from cchinchilla-dev:feat/add_override_decorators_to_loggingplugin_4496 142ed87 PiperOrigin-RevId: 875811966
|
Thank you @cchinchilla-dev for your contribution! 🎉 Your changes have been successfully imported and merged via Copybara in commit 65d9a72. Closing this PR as the changes are now in the main branch. |
|
Hi @cchinchilla-dev , its merged. |
Merge google#4572 ### Link to Issue or Description of Change **1. Link to an existing issue (if applicable):** - Closes: google#4496 **2. Or, if no issue exists, describe the change:** **Problem:** `LoggingPlugin` overrides 12 callback methods from `BasePlugin` but none use the `@override` decorator. Every other plugin in the package (`DebugLoggingPlugin`, `ReplayPlugin`, `RecordingsPlugin`, `EnsureRetryOptionsPlugin`, `_RequestIntercepterPlugin`) already follows this practice. With `mypy --strict` enabled in `pyproject.toml`, missing `@override` means renamed or removed base-class methods would be silently missed in `LoggingPlugin` while being caught everywhere else. **Solution:** Import `override` from `typing_extensions` and decorate all 12 overridden callbacks. Purely additive: one import line and 12 decorators. No behavioral, API, or runtime change. ### Testing Plan **Unit Tests:** - [ ] I have added or updated unit tests for my change. - [x] All unit tests pass locally. No new tests are required — `@override` is a static-analysis-only decorator with no runtime effect. Ran `mypy` on the file before and after the change: same preexisting warnings, no new errors introduced. CI will validate via the existing test suite and linting checks. **Manual End-to-End (E2E) Tests:** Not applicable. This change adds only decorators with no runtime behavior. Verified by comparing `mypy` output before and after — no new errors. ### Checklist - [x] I have read the [CONTRIBUTING.md](https://github.com/google/adk-python/blob/main/CONTRIBUTING.md) document. - [x] I have performed a self-review of my own code. - [x] I have commented my code, particularly in hard-to-understand areas. - [ ] I have added tests that prove my fix is effective or that my feature works. - [x] New and existing unit tests pass locally with my changes. - [ ] I have manually tested my changes end-to-end. - [ ] Any dependent changes have been merged and published in downstream modules. ### Additional context I am the author of the original issue (google#4496). A previous PR (google#4544) was opened but is pending clarification, so I'm submitting this complete PR as the original issue author. COPYBARA_INTEGRATE_REVIEW=google#4572 from cchinchilla-dev:feat/add_override_decorators_to_loggingplugin_4496 142ed87 PiperOrigin-RevId: 875811966
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
LoggingPluginoverrides 12 callback methods fromBasePluginbut none use the@overridedecorator. Every other plugin in the package (DebugLoggingPlugin,ReplayPlugin,RecordingsPlugin,EnsureRetryOptionsPlugin,_RequestIntercepterPlugin) already follows this practice. Withmypy --strictenabled inpyproject.toml, missing@overridemeans renamed or removed base-class methods would be silently missed inLoggingPluginwhile being caught everywhere else.Solution:
Import
overridefromtyping_extensionsand decorate all 12 overridden callbacks. Purely additive: one import line and 12 decorators. No behavioral, API, or runtime change.Testing Plan
Unit Tests:
No new tests are required —
@overrideis a static-analysis-only decorator with no runtime effect. Ranmypyon the file before and after the change: same preexisting warnings, no new errors introduced. CI will validate via the existing test suite and linting checks.Manual End-to-End (E2E) Tests:
Not applicable. This change adds only decorators with no runtime behavior. Verified by comparing
mypyoutput before and after — no new errors.Checklist
Additional context
I am the author of the original issue (#4496). A previous PR (#4544) was opened but is pending clarification, so I'm
submitting this complete PR as the original issue author.