mypy icon indicating copy to clipboard operation
mypy copied to clipboard

Add common issue docs for PEP585 syntax update

Open LefterisJP opened this issue 2 years ago • 10 comments

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

LefterisJP avatar Dec 04 '22 13:12 LefterisJP

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

LefterisJP avatar Dec 04 '22 15:12 LefterisJP

The off by one error is funny 🤣

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

AlexWaygood avatar Dec 04 '22 15:12 AlexWaygood

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.

LefterisJP avatar Dec 04 '22 16:12 LefterisJP

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`

AlexWaygood avatar Dec 04 '22 16:12 AlexWaygood

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.

LefterisJP avatar Dec 04 '22 17:12 LefterisJP

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 avatar Dec 04 '22 17:12 AlexWaygood

Looks great now! Just two minor points:

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

LefterisJP avatar Dec 04 '22 17:12 LefterisJP

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.

JelleZijlstra avatar Dec 06 '22 03:12 JelleZijlstra

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:

  1. references to PEP584?
  2. The recommendation to use typing.Type ?

LefterisJP avatar Dec 07 '22 17:12 LefterisJP

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.

JelleZijlstra avatar Dec 20 '22 21:12 JelleZijlstra