Skip to content
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

core: Add ruff rules ANN (type annotations) #29271

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

cbornet
Copy link
Collaborator

@cbornet cbornet commented Jan 17, 2025

See https://docs.astral.sh/ruff/rules/#flake8-annotations-ann
The interest compared to only mypy is that ruff is very fast at detecting missing annotations.

ANN101 and ANN102 are deprecated so we ignore them
ANN401 (no Any type) ignored to be in sync with mypy config

Copy link

vercel bot commented Jan 17, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
langchain ⬜️ Ignored (Inspect) Visit Preview Jan 25, 2025 5:07pm

if instance is not None or owner is not None:
emit_warning()
if self.fget is None:
Copy link
Collaborator Author

@cbornet cbornet Jan 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other possibility could be to call super().__get__ ?

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs labels Jan 17, 2025
@@ -2186,7 +2186,7 @@ class Foo(BaseModelV2):
)

@tool(args_schema=Foo)
def foo(x): # type: ignore[no-untyped-def]
def foo(x: list[int]) -> list[int]:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably reasonable to not update this type signature -- this test is attempting to verify that all the information is getting picked up from the args_schema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed!
Done in 454b292

@dosubot dosubot bot added the lgtm PR looks good. Use to confirm that a PR is ready for merging. label Jan 20, 2025
@cbornet cbornet force-pushed the ruff-core-ann branch 2 times, most recently from 454b292 to eda139c Compare January 21, 2025 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm PR looks good. Use to confirm that a PR is ready for merging. 🤖:nit Small modifications/deletions, fixes, deps or improvements to existing code or docs size:M This PR changes 30-99 lines, ignoring generated files.
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

2 participants