-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Add info about classes and types to 'getting started' docs #6557
Add info about classes and types to 'getting started' docs #6557
Conversation
While helping somebody use mypy, it came to my attention that our docs never actually quite explain how classes and types interact. This PR adds a new section named "Types and classes" to the "Getting started" docs that tries to fix this. This section focuses on introducing two ideas: 1. That you can use any Python class as a type 2. That if a variable has type T, it can also accept any instances of *subclasses* of type T. The examples also give us an opportunity to sneak in a brief mini-lesson on how to add type hints to classes, and what sorts of things do and do not get inferred. I also rewrote the next section about stubs/typeshed partly so it would dovetail better with this new section but also partly because I always felt that section was a little too dense and rushed. Three other minor changes made: - I added an example about sqlalchemy and sqlalchemy-stubs to one of the other pages. - I removed a line directing people to the mypy issue tracker if they have questions. - I dropped the term "library stubs" in favor of just "stub files".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, just couple random thoughts.
This behavior is actually a fundamental aspect of the PEP 484 type system: when | ||
we annotate some variable with a type ``T``, we are actually telling mypy that | ||
variable can be assigned an instance of ``T``, or an instance of a *subclass* of ``T``. | ||
The same rule applies to type hints on parameters or fields. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we mention that mypy enforces LSP to make sure this is actually safe (maybe in a footnote)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to mention that in this article, since this is a beginner's tutorial.
docs/source/getting_started.rst
Outdated
1. Look for a separate package containing stubs for that library. For example, | ||
if you want type hints for the `sqlalchemy <https://www.sqlalchemy.org/>`_ | ||
library, you can install | ||
`sqlalchemy-stubs <https://pypi.org/project/sqlalchemy-stubs/>`_. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also mention other notable stubs projects? This link appears twice (here and in the section on running mypy), maybe replace one of them with e.g. django-stubs
?
@JukkaL are you going to review this PR? If no, I can do a more thorough review. |
Hey, @Michael0x2a, if you rebase this, I'll review it. |
Any progress on this PR? |
These still look useful improvements, and I can review this if the merge conflicts get resolved. |
docs/source/getting_started.rst
Outdated
the types are: it'll assume the entire library is | ||
:ref:`dynamically typed <dynamic-typing>` and report an error | ||
whenever you import the library. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JukkaL I tried to merge the conflicts. But I am not sure about this parts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section (from lines 452-487) should be moved to running_mypy.rst
, or maybe just cut altogether. With the changes made in this PR, it's a very different topic to the rest of the article, and I don't think we need it here since it's covered in depth elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the changes by Michael. But I keep the current example since it's quite useful for beginners IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for cutting this section entirely. At this point, typeshed is good enough that users don't need to know at all about typeshed and beginner users don't really need to know about stubs to get started
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a really nice PR! Some thoughts below :)
docs/source/getting_started.rst
Outdated
# In this case, mypy can't infer the exact type of this field | ||
# based on the information available in this constructor, so we | ||
# need to add a type annotation. | ||
self.audit_log: List[str] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not massively keen on an example that requires a three-line comment, especially since this is a beginner's tutorial. Can we think of a different example here, that doesn't involve having to instantiate an empty list? (I'm also trying to think of one.)
In any event, we should use PEP 585 syntax here :)
# In this case, mypy can't infer the exact type of this field | |
# based on the information available in this constructor, so we | |
# need to add a type annotation. | |
self.audit_log: List[str] = [] | |
# In this case, mypy can't infer the exact type of this field | |
# based on the information available in this constructor, so we | |
# need to add a type annotation. | |
self.audit_log: list[str] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a transaction count as an int?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it's not really a big deal since typing an empty list is a frequent case (personal feeling).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy would be able to infer an int
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to keep the list[str]
but remove the comments since the rule has been introduced in previous section
docs/source/getting_started.rst
Outdated
the types are: it'll assume the entire library is | ||
:ref:`dynamically typed <dynamic-typing>` and report an error | ||
whenever you import the library. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section (from lines 452-487) should be moved to running_mypy.rst
, or maybe just cut altogether. With the changes made in this PR, it's a very different topic to the rest of the article, and I don't think we need it here since it's covered in depth elsewhere.
(BTW @97littleleaf11, I'm only a triager so I can review, but I can't edit the PR :) |
Co-authored-by: AlexWaygood <[email protected]>
@AlexWaygood Thanks for your reviews! It's quite an old PR and let's make it merged soon. I fixed the conflicts and added back some changes made after this PR. Could you take another round of check and merge it if possible? |
I don't have merge privileges, but I'd be happy to do another review at some point in the next few days! :) |
docs/source/getting_started.rst
Outdated
# In this case, mypy can't infer the exact type of this field | ||
# based on the information available in this constructor, so we | ||
# need to add a type annotation. | ||
self.audit_log: List[str] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy would be able to infer an int
docs/source/getting_started.rst
Outdated
the types are: it'll assume the entire library is | ||
:ref:`dynamically typed <dynamic-typing>` and report an error | ||
whenever you import the library. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vote for cutting this section entirely. At this point, typeshed is good enough that users don't need to know at all about typeshed and beginner users don't really need to know about stubs to get started
Thanks everyone for getting this over the line! |
While helping somebody use mypy, it came to my attention that our docs never actually quite explain how classes and types interact.
This PR adds a new section named "Types and classes" to the "Getting started" docs that tries to fix this.
This section focuses on introducing two ideas:
The examples also give us an opportunity to sneak in a brief mini-lesson on how to add type hints to classes, and what sorts of things do and do not get inferred.
I also rewrote the next section about stubs/typeshed partly so it would dovetail better with this new section but also partly because I always felt that section was a little too dense and rushed.
Three other minor changes made:
I added an example about sqlalchemy and sqlalchemy-stubs to one of the other pages.
I removed a line directing people to the mypy issue tracker if they have questions.
I dropped the term "library stubs" in favor of just "stub files".