Add type annotations for rings and parents #41232
Conversation
tobiasdiez
force-pushed
the
typing-catrings
branch
2 times, most recently
from
c7768dd to
fab7698
Compare
tobiasdiez
force-pushed
the
typing-catrings
branch
from
fab7698 to
315aa5c
Compare
vincentmacri
left a comment
vincentmacri
left a comment
There was a problem hiding this comment.
Good overall. Just a few questions to make sure I understand this right.
My biggest comment is more of a question. I believe type checkers try to enforce some kind of Liskov substitution principle stuff, which would mean that annotations like def __hash__(self) -> int: are redundant if that annotation is already present in SageObject. So some of the type annotations would be redundant if I'm correct about that.
|
|
||
| def __init__(self, parent: Any, codomain: Parent[Any] | None = ...) -> None: ... | ||
| def __copy__(self) -> Self: ... | ||
| def parent(self) -> Any: ... |
There was a problem hiding this comment.
Why isn't this annotated to return Parent?
| DomainElementT_contra = TypeVar("DomainElementT_contra", contravariant=True) | ||
| CodomainElementT_co = TypeVar("CodomainElementT_co", covariant=True) |
There was a problem hiding this comment.
I've seen contravariant and covariant in the Python docs before but haven't really seen them used. For the sake of helping me review this, could you briefly explain what these are doing?
| def __reduce__(self) -> tuple[Any, tuple[Any, ...]]: ... | ||
| def _repr_type(self) -> str: ... | ||
| def _repr_defn(self) -> str: ... | ||
| def _repr_(self) -> str: ... |
There was a problem hiding this comment.
Shouldn't the type annotation for _repr_ (and other methods like __hash__) be inherited from SageObject? I though type checkers enforced that through the Liskov substitution principle (at least some do), which would mean that this annotation is redundant.
If I'm correct about the behaviour of type annotations with inheritance, that would mean that we would get the most value for the amount of work by annotating classes higher up in the inheritance hierarchy (like you've done in this PR).
|
note that "ParentWithGens" is something we should get rid of. Efforts in this direction are currentl blocked at the ticket about moving "fraction_field" method #40213 |
3 participants
Add a bit more typing annotations/stubs for a few fundamental objects (with the goal to make typing of the higher level Python interface easier). They are not meant to be 100% accurate and contain a few to many
Anys for my taste, but it should serve as a good start.📝 Checklist
⌛ Dependencies