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

Improve DataclassInstance; add more tests #10

Merged
merged 24 commits into from
Jul 23, 2023
Merged

Conversation

AlexWaygood
Copy link
Collaborator

This renames DataclassInstance to DataclassLike, and brings the runtime implementation inline with what I proposed in python/cpython#102933.

I've also copied over the tests from python/cpython#102933, and some of the static tests we have in typeshed.

tests/test_dataclasslike.py Outdated Show resolved Hide resolved
useful_types/experimental.py Outdated Show resolved Hide resolved
from typing_extensions import LiteralString, TypeAlias

ExcInfo: TypeAlias = Tuple[type[BaseException], BaseException, TracebackType]
ExcInfo: TypeAlias = Tuple[Type[BaseException], BaseException, TracebackType]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI useful_types.experimental currently fails to import on py38 due to this!

static_tests/test_dataclasslike.py Show resolved Hide resolved
class DataclassLikeTests(unittest.TestCase):
def test_basics(self):
# As an abstract base class for all dataclasses,
# it makes sense for DataclassLike to also be considered a dataclass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does it? DataclassLike (the class) is not itself a dataclass, just as we have is_protocol(Protocol) return False.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DataclassLike (the class) is not itself a dataclass

Well, it is -- it's decorated with @dataclass! And this holds true for all other classes decorated with @dataclass, so I don't see why it shouldn't for DataclassLike?

>>> import dataclasses
>>> @dataclasses.dataclass
... class X: ...
...
>>> dataclasses.is_dataclass(X)
True

just as we have is_protocol(Protocol) return False

Protocol and TypedDict are pretty special. Neither of them is an ABC/protocol; they're factories for creating special kinds of types.

But DataclassLike is a protocol, and I think it should act like any other protocol:

>>> import typing
>>> issubclass(typing.SupportsIndex, typing.SupportsIndex)
True
>>> import collections.abc
>>> issubclass(collections.abc.Iterable, collections.abc.Iterable)
True
>>> callable(collections.abc.Callable)
True

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see, the runtime version does have the @dataclass decorator, but the if TYPE_CHECKING version doesn't, and that's the one I was looking at.

I'm talking about the behavior of is_dataclass() here, not issubclass(), so most of the examples you cite don't really apply. collections.abc.Callable really is callable.

Copy link
Collaborator Author

@AlexWaygood AlexWaygood Jun 21, 2023

Choose a reason for hiding this comment

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

I'm talking about the behavior of is_dataclass() here, not issubclass(), so most of the examples you cite don't really apply. collections.abc.Callable really is callable.

is_dataclass(X) asks, "Does the class X have the necessary qualities to be considered a dataclass?". issubclass(X, collections.abc.Iterable) asks, "Does the class X have the necessary qualities to be considered an iterable?" I view the questions as analogous, even if they're posed in slightly different ways at the Python level.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's more like these:

>>> inspect.isfunction(types.FunctionType)
False
>>> inspect.iscode(types.CodeType)
False
>>> inspect.ismodule(types.ModuleType)
False

Copy link
Collaborator Author

@AlexWaygood AlexWaygood Jun 21, 2023

Choose a reason for hiding this comment

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

is_dataclass(X) returns True even if X is a dataclass instance or a dataclass class. That's a pretty odd choice for an API for is_dataclass. It feels dataclasses.is_dataclass would be a much closer analogy to those inspect.is* functions if it returned False for dataclass classes, and only returned True for dataclass instances.

Copy link
Owner

Choose a reason for hiding this comment

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

Just looking at this, sorry, I'm confused. Which version of DataclassLike has the @dataclass decorator?

Since I'm not sure I understand the situation, I don't have an opinion yet. We currently disallow subclassing, which is wise, if we also make the protocol not runtime checkable then there aren't really any runtime use cases for this, which could render a decision here somewhat academic.

Copy link
Collaborator Author

@AlexWaygood AlexWaygood Jun 26, 2023

Choose a reason for hiding this comment

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

Currently no version of DataclassLike has the @dataclass decorator, because I addressed Jelle's other comment elsewhere where he said he wanted the runtime implementation and the static implementation to be as similar as possible :) An earlier version had the decorator on the runtime implementation, but not the static implementation.

dataclasses.is_dataclass(DataclassLike) still returns True, however, because all the function looks for is the presence of a __dataclass_fields__ class variable, which DataclassLike has.

if we also make the protocol not runtime checkable then there aren't really any runtime use cases for this, which could render a decision here somewhat academic.

That's a good point actually — maybe we should just do that, on the grounds that dataclasses already has ways to determine whether or not something is a dataclass at runtime (the is_dataclass function)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The PR no longer proposes making it runtime-checkable!

useful_types/experimental.py Outdated Show resolved Hide resolved
AlexWaygood added a commit that referenced this pull request Jun 21, 2023
Splitting this out from #10 so we don't get a confusing `git blame`
JelleZijlstra pushed a commit that referenced this pull request Jun 21, 2023
Splitting this out from #10 so we don't get a confusing `git blame`
@AlexWaygood
Copy link
Collaborator Author

@JelleZijlstra
Copy link
Collaborator

I feel like you need an Experts entry in the devguide for "obscure Protocol bugs".

@AlexWaygood
Copy link
Collaborator Author

I feel like you need an Experts entry in the devguide for "obscure Protocol bugs".

yes

@AlexWaygood
Copy link
Collaborator Author

at least this bug doesn't depend on precisely when the GC runs, things are improving

@AlexWaygood
Copy link
Collaborator Author

CI is all green now; we just have to resolve our disagreement in #10 (comment)

@AlexWaygood AlexWaygood merged commit d900708 into main Jul 23, 2023
@AlexWaygood AlexWaygood deleted the dataclass-proto branch July 23, 2023 00:46
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.

3 participants