mypy
mypy copied to clipboard
Add common issue docs for PEP585 syntax update
As discussed with @AlexWaygood in https://github.com/python/mypy/issues/14245#issuecomment-1336397899 adding a common issue entry for the problem of hitting overshadowing a built-in keyword when upgrading to PEP584 syntax
Thanks! I've left a few suggestions below.
Main points:
- It's PEP 585, not PEP 584 :)
- Stick to <=80 characters per line where possible.
type
is a builtin name, but it's not a keyword- Use Sphinx roles to add links where appropriate
Thanks I addressed all your points. Didn't know of the sphinx roles and links. The off by one error is funny :rofl:
I left you two extra questions
The off by one error is funny 🤣
You need to update the title of the PR as well ;)
You need to update the title of the PR as well ;)
Done!
Thanks! A few more points:
Addressed them and pushed.
One question from my side. Here I asked you if you know the sphinx link to type for the docs. Any idea? This does not seem to render properly. But I see the same problem in other places in the docs as per my comment.
One question from my side. Here I asked you if you know the sphinx link to type for the docs. Any idea? This does not seem to render properly. But I see the same problem in other places in the docs as per my comment.
Hmm, maybe try
:py:class:`type`
One question from my side. Here I asked you if you know the sphinx link to type for the docs. Any idea? This does not seem to render properly. But I see the same problem in other places in the docs as per my comment.
Hmm, maybe try
:py:class:`type`
Yeah! That seems to work! Generates a proper link to: https://docs.python.org/3/library/functions.html#type
I pushed it to the PR.
Follow up question then. There is one more place in the docs where this needs fixing. Should I do it in this PR and a separate commit? A different PR? I am not sure what workflow you guys prefer.
Follow up question then. There is one more place in the docs where this needs fixing. Should I do it in this PR and a separate commit? A different PR? I am not sure what workflow you guys prefer.
That would be a very welcome fix, but definitely best to do it as a separate PR, since it's a different issue to the thing being tackled here :)
Looks great now! Just two minor points:
Addressed both. And also opened that follow-up PR.
It's a bit weird to frame this around PEP 585, when PEP 585 doesn't really do anything special here: you can run into the exact same problem with types like int
and str
that have always been usable as annotations. Indeed there is already a section in "Common issues" about this: https://mypy.readthedocs.io/en/stable/common_issues.html#dealing-with-conflicting-names. So if anything, we should enhance that section.
I would also recommend removing the recommendation to use typing.Type
. One of the effects of PEP 585 is that things like "type" and "list" are no longer special in the sense that they have a separate typing alter ego. That's good for learning because users no longer have to remember which names are special like that. So we shouldn't point users to typing.Type
; people working in a post-PEP 585 world shouldn't need to know it even exists.
It's a bit weird to frame this around PEP 585, when PEP 585 doesn't really do anything special here: you can run into the exact same problem with types like
int
andstr
that have always been usable as annotations. Indeed there is already a section in "Common issues" about this: https://mypy.readthedocs.io/en/stable/common_issues.html#dealing-with-conflicting-names. So if anything, we should enhance that section.I would also recommend removing the recommendation to use
typing.Type
. One of the effects of PEP 585 is that things like "type" and "list" are no longer special in the sense that they have a separate typing alter ego. That's good for learning because users no longer have to remember which names are special like that. So we shouldn't point users totyping.Type
; people working in a post-PEP 585 world shouldn't need to know it even exists.
Thank you for the review @JelleZijlstra @AlexWaygood. So how would you like me to proceed with the PR to get it merged? Add the section as is under the section you recommended removing:
- references to PEP584?
- The recommendation to use
typing.Type
?
Yes, I'd recommend adding a shortened text under the existing section. It's probably helpful to use type
as the main example since that's in my experience the most common name for which this problem comes up.