ci: add typecheck #39
Conversation
Summary of ChangesHello @matiasanaya, 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 integrates mypy for static type checking across the project, aiming to enhance code quality and reduce runtime errors. A significant part of this effort involved restructuring the 'types' module into 'core/types' to address mypy's import resolution, necessitating widespread updates to import paths. The changes also include adding mypy as a development dependency and a dedicated script for running type checks. Highlights
Ignored Files
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
|
1a333de to
aa8b3f4
Compare
There was a problem hiding this comment.
Code Review
This pull request is a great step towards improving code quality by introducing mypy for static type checking. The main change involves refactoring the project structure by moving the types package into core, and updating all import paths accordingly. The configuration changes in pyproject.toml and lock files to add and configure mypy are also correct.
I've found a minor maintainability issue regarding redundant local imports in core/utils.py. Consolidating these at the top of the file would improve readability. Overall, the changes are well-executed and align with the goal of adding type checking.
python/x402_a2a/core/utils.py
Outdated
| # Ensure task has a status message for metadata | ||
| if not hasattr(task.status, "message") or not task.status.message: | ||
| from ..types import Message | ||
| from ..core.types import Message |
There was a problem hiding this comment.
The Message type is already imported at the top of this file (line 21), making this local import redundant. The TextPart import on the next line is also redundant as it's imported on line 30.
This pattern of redundant local imports occurs on lines 218-219, 246-247, and 283-284. Consolidating imports at the top of the file improves readability and maintainability.
You can refactor this block to:
if not hasattr(task.status, "message") or not task.status.message:
task.status.message = Message(
messageId=f"{task.id}-status",
role="agent",
parts=[
TextPart(kind="text", text="Payment is required for this service.")
],
metadata={},
)aa8b3f4 to
07f8462
Compare
lingzhong
left a comment
There was a problem hiding this comment.
Thank you @matiasanaya for proposing this, we're well aware!
Two notes:
- We're currently focusing on refining the protocol flow so anticipate volatile code changes in both the protocol and the demo paths. Timing-wise it may be better to start enforcing type checks after upcoming volatility settles.
- We certainly to want to try and keep the
typesnaming.
Looking forward to these.
Agreed What's the best way to contribute right now given this? Don't want to unnecessarily add extra burden, though I'm trying to integrate x402 payments over A2A to a production agent and I have several thoughts / concerns. |
Appreciate your thoughtfulness! Your feedback around production integration will be invaluable, especially pain points and concerns. Can you open new issues for these concerns? Ideally a separate issue for each independent concern or idea, so appropriate folks can be pulled into the discussions. Hopefully this will reduce significant PR authoring and review taking up cycles prematurely. |
I believe we should add
mypyfor typechecking. If this is valuable, I'd need to fix / silence the following errors as part of this PR:As a note, I've had to rename
typestotype_defsbecausemypywouldn't run based ontypesshadowing the std lib. There is probably a more elegant solution.