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

Support dataclass transform #2049

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

Conversation

Morikko
Copy link

@Morikko Morikko commented Feb 15, 2025

Goal

Support dataclass transform as specified: https://typing.readthedocs.io/en/latest/spec/dataclasses.html#dataclass-transform

Motivation

To have signature completions when using libraries like Pydantic or SQLalchemy.

Technical details

  1. Python Support
# 3.11+
from typing import dataclass_transform
# <3.11
from typing_extensions import dataclass_transform
  1. The decorator usage is very similar to the dataclasses.dataclass

  2. With a difference though:

If dataclass_transform is applied to a class, dataclass-like semantics will be assumed for any class that directly or indirectly derives from the decorated class or uses the decorated class as a metaclass. Attributes on the decorated class and its base classes are not considered to be fields.

@Morikko
Copy link
Author

Morikko commented Feb 15, 2025

For now, the proposal is a PoC before continuing I would like to know:

  1. If there is any chance to merge it. I hope so according to this comment:

I also agree with your comment in the PR that we should probably focus on PEP 681 dataclass_transforms if we want to go further.

#1835 (comment)

  1. Because of the technical point 3., the implementation is more complex and I had to tweak klass.py. I would like to know if the design/implementation is going towards the right direction.

Also, I am not very familiar with jedi codebase so any feedback is highly appreciable.

@Morikko
Copy link
Author

Morikko commented Feb 15, 2025

 jedi/inference/value/klass.py:255: error: "ClassMixin" has no attribute "get_metaclasses"  [attr-defined]
Found 1 error in 1 file (checked 86 source files)

Interesting, the method get_signatures from the same class is also using it but without a concern.

filter_ = cls.as_context().get_global_filter()
# .values ordering is not guaranteed, at least not in
# Python < 3.6, when dicts where not ordered, which is an
# implementation detail anyway.
Copy link
Owner

Choose a reason for hiding this comment

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

Python < 3.6 is not supported anymore.

@davidhalter
Copy link
Owner

Thanks a lot for taking this on!

If there is any chance to merge it.

100%. In general my acceptance of changes mostly depends on quality and test coverage. This looks very good so far. Feel free to "finish" it as far as you can and then I'll review it. I think there might be a few minor changes or discussions, but they shouldn't cost you a lot of time. Also don't waste too much time in making Mypy happy. You can also skip some things with # type: ignore, if it's really not easy to type it. In general the Jedi code base is only typed in very little parts. Typing things is definitely a good thing, but some old code is very hard to type, because it works a lot with kind of obscure duck typing.

the implementation is more complex and I had to tweak klass.py. I would like to know if the design/implementation is going towards the right direction.

The complexity is unfortunately expected, with pretty much all typing changes. This change is on the very light side with only minor changes. My Mypy in Rust rewrite had to do way more to implement dataclass_transform (for doing proper type checking). Also note that this is a very low impact change, because it doesn't affect type inference and therefore won't affect performance.

Happy to review it thoroughly once you think this is ready. I would be happy to see at least some of the CI green. But I'm happy to help out if there are issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants