Add common issue docs for PEP585 syntax update #14246

LefterisJP · 2022-12-04T13:09:08Z

As discussed with @AlexWaygood in #14245 (comment) adding a common issue entry for the problem of hitting overshadowing a built-in keyword when upgrading to PEP584 syntax

AlexWaygood

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

docs/source/common_issues.rst

LefterisJP · 2022-12-04T15:39:54Z

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 🤣

I left you two extra questions

AlexWaygood · 2022-12-04T15:57:12Z

The off by one error is funny 🤣

You need to update the title of the PR as well ;)

AlexWaygood

Thanks! A few more points:

docs/source/common_issues.rst

LefterisJP · 2022-12-04T16:37:05Z

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.

AlexWaygood · 2022-12-04T16:42:24Z

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`

LefterisJP · 2022-12-04T17:00:20Z

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.

AlexWaygood · 2022-12-04T17:02:32Z

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 :)

AlexWaygood

Looks great now! Just two minor points:

docs/source/common_issues.rst

@AlexWaygood

As discussed with @AlexWaygood in python#14245 (comment) adding a common issue entry for the problem of hitting overshadowing a built-in keyword when upgrading to PEP585 syntax

LefterisJP · 2022-12-04T17:15:38Z

Looks great now! Just two minor points:

Addressed both. And also opened that follow-up PR.

AlexWaygood

LGTM!

JelleZijlstra · 2022-12-06T03:45:03Z

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.

LefterisJP · 2022-12-07T17:05:49Z

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.

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 ?

JelleZijlstra · 2022-12-20T21:54:07Z

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.

Switch :py:func:type to py:class:type in type_narrowing.rst. The former does not render properly in the docs This is a tiny follow up from #14246 (comment)

AlexWaygood self-requested a review December 4, 2022 13:10

AlexWaygood reviewed Dec 4, 2022

View reviewed changes

LefterisJP force-pushed the docs_for_pep584_syntax_upgrade branch from 9bf1d9a to b595128 Compare December 4, 2022 15:38

AlexWaygood reviewed Dec 4, 2022

View reviewed changes

docs/source/common_issues.rst Outdated Show resolved Hide resolved

docs/source/common_issues.rst Outdated Show resolved Hide resolved

docs/source/common_issues.rst Outdated Show resolved Hide resolved

docs/source/common_issues.rst Outdated Show resolved Hide resolved

LefterisJP changed the title ~~Add common issue docs for PEP584 syntax upgrade~~ Add common issue docs for PEP585 syntax update Dec 4, 2022

LefterisJP force-pushed the docs_for_pep584_syntax_upgrade branch from b595128 to a9e081d Compare December 4, 2022 16:35

LefterisJP force-pushed the docs_for_pep584_syntax_upgrade branch from a9e081d to 792fb72 Compare December 4, 2022 16:58

AlexWaygood reviewed Dec 4, 2022

View reviewed changes

docs/source/common_issues.rst Outdated Show resolved Hide resolved

docs/source/common_issues.rst Outdated Show resolved Hide resolved

LefterisJP mentioned this pull request Dec 4, 2022

Switch :py:func:type to py:class:type in type_narrowing.rst #14253

Merged

Add common issue docs for PEP585 syntax upgrade

281d279

As discussed with @AlexWaygood in python#14245 (comment) adding a common issue entry for the problem of hitting overshadowing a built-in keyword when upgrading to PEP585 syntax

LefterisJP force-pushed the docs_for_pep584_syntax_upgrade branch from 792fb72 to 281d279 Compare December 4, 2022 17:15

AlexWaygood approved these changes Dec 4, 2022

View reviewed changes

AlexWaygood requested a review from JelleZijlstra December 4, 2022 17:16

AlexWaygood added the documentation label Dec 11, 2022

JelleZijlstra removed their request for review December 20, 2022 21:54

hauntsaninja pushed a commit that referenced this pull request Dec 20, 2022

Fix RST markup in type_narrowing.rst (#14253)

cb1d1a0

Switch :py:func:type to py:class:type in type_narrowing.rst. The former does not render properly in the docs This is a tiny follow up from #14246 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add common issue docs for PEP585 syntax update #14246

Add common issue docs for PEP585 syntax update #14246

LefterisJP commented Dec 4, 2022

AlexWaygood left a comment

LefterisJP commented Dec 4, 2022

AlexWaygood commented Dec 4, 2022

AlexWaygood left a comment

LefterisJP commented Dec 4, 2022

AlexWaygood commented Dec 4, 2022 •

edited

Loading

LefterisJP commented Dec 4, 2022 •

edited

Loading

AlexWaygood commented Dec 4, 2022

AlexWaygood left a comment

LefterisJP commented Dec 4, 2022

AlexWaygood left a comment

JelleZijlstra commented Dec 6, 2022

LefterisJP commented Dec 7, 2022

JelleZijlstra commented Dec 20, 2022

Add common issue docs for PEP585 syntax update #14246

Are you sure you want to change the base?

Add common issue docs for PEP585 syntax update #14246

Conversation

LefterisJP commented Dec 4, 2022

AlexWaygood left a comment

Choose a reason for hiding this comment

LefterisJP commented Dec 4, 2022

AlexWaygood commented Dec 4, 2022

AlexWaygood left a comment

Choose a reason for hiding this comment

LefterisJP commented Dec 4, 2022

AlexWaygood commented Dec 4, 2022 • edited Loading

LefterisJP commented Dec 4, 2022 • edited Loading

AlexWaygood commented Dec 4, 2022

AlexWaygood left a comment

Choose a reason for hiding this comment

LefterisJP commented Dec 4, 2022

AlexWaygood left a comment

Choose a reason for hiding this comment

JelleZijlstra commented Dec 6, 2022

LefterisJP commented Dec 7, 2022

JelleZijlstra commented Dec 20, 2022

AlexWaygood commented Dec 4, 2022 •

edited

Loading

LefterisJP commented Dec 4, 2022 •

edited

Loading