bpo-43987: Add "Annotations Best Practices" HOWTO doc.#25746
bpo-43987: Add "Annotations Best Practices" HOWTO doc.#25746larryhastings merged 11 commits intopython:masterfrom
Conversation
JelleZijlstra
left a comment
There was a problem hiding this comment.
Thanks, this is great! A couple of suggestions:
- It would be useful to add a link in more places to make sure it's findable. You wrote in the issue that "it just didn't seem like a good idea", but I'm not sure why; these other places seem more likely to be seen by users than the glossary or the docs for the new function
inspect.get_annotations. - I'm worried that people will come to this page expecting it to be about how to write type hints for their code (there are a lot more people writing type hints than writing code that accesses
__annotations__at runtime). It'd be good to add links to some document explaining how to do that, perhaps the mypy docs or the docs for thetypingmodule.
Doc/howto/annotations.rst
Outdated
| a:int = 3 | ||
| b:str = 'abc' |
There was a problem hiding this comment.
| a:int = 3 | |
| b:str = 'abc' | |
| a: int = 3 | |
| b: str = 'abc' |
This is the format recommended by https://www.python.org/dev/peps/pep-0008/#variable-annotations
Doc/howto/annotations.rst
Outdated
| for you. Un-stringizing is a complicated process, and | ||
| it's best to let :func:`inspect.get_annotations` do it | ||
| for you. |
There was a problem hiding this comment.
| for you. Un-stringizing is a complicated process, and | |
| it's best to let :func:`inspect.get_annotations` do it | |
| for you. | |
| for you. |
This sentence is just repetitive
There was a problem hiding this comment.
I'm also not sure I agree with the recommendation. There's a few plausible scenarios where it won't work:
- The annotation uses a name defined in a nested scope that's no longer accessible (one of the scenarios that PEP 649 would fix)
- The annotation uses a name defined in
if TYPE_CHECKING - The annotation uses PEP 604 syntax
int | strandfrom __future__ import annotationsbefore Python 3.10
In such cases, eval will throw an error at runtime, but if you try hard enough you can still extract useful information. In my own tool (pyanalyze) I instead ast.parse string annotations and evaluate them manually.
I would recommend at least adding a warning that eval() may throw for some annotations that static type checkers will process correctly.
There was a problem hiding this comment.
I followed all your suggestions, except adding references from datamodel.rst. Again, I'm not sure how to work it in in a way that reads well. Can you give a concrete example of how you'd add a reference to this HOWTO?
p.s. I also found one or two spots where I added a contraction. When I get exacting, I tend to stop using contractions, and which I think makes my text sound stiff.
p.p.s. Did I forget any quirks?
There was a problem hiding this comment.
Can you give a concrete example of how you'd add a reference to this HOWTO?
How about adding "For more details on how to use this attribute, see " to the cell for __annotations__ in the table under https://docs.python.org/3/reference/datamodel.html#the-standard-type-hierarchy ?
|
I gave it a shot, see what you think. I'm still on the fence. I like having the references there but it still seems to be sticking out a little. (Then again, |
Doc/reference/datamodel.rst
Outdated
| that are statically linked into the interpreter; for extension modules | ||
| loaded dynamically from a shared library, it is the pathname of the shared | ||
| library file. | ||
| library file; :attr:`__annotations__` (optional) is a dictionary containing |
There was a problem hiding this comment.
This might work better as a bulleted list, instead of a long string of semicolon-separated fragments.
There was a problem hiding this comment.
Though I take neither credit nor blame for the original formatting, and such improvements are verging on mission creep, I'll still give it a go. I'm going to try a "definition list" first and see if I can make that work; failing that, a bulleted list as you suggest. Either way I'll do that in both spots, modules and classes.
There was a problem hiding this comment.
I quite like how that turned out! I invite you to see if you agree.
It makes me wonder if I should now touch up the Special read-only attribute: paragraph in the Modules section somehow. On the other hand, there are a bunch of previous instances of special read-only attributes sprinkled through the preceding text, so it's not completely out of line.
JelleZijlstra
left a comment
There was a problem hiding this comment.
I agree, this looks a lot more readable! Left a few tiny suggestions.
|
I remembered tonight that "What's New" no longer has a designated curator, but rather is self-serve. By adding my two recent contributions to (C)Python, I naturally had two more spots I could crowbar in references to "Annotations Best Practices". See what you think! |
This commit message is way, way longer than the diff!
|
I realize it seems like an unnecessary formality at this point, but I would still prefer (and enjoy!) a LGTM review here. |
|
I had a comment above on adding more pitfalls about eval that I think would still be useful. I can try to write up some text myself (but not right now). |
|
Oh, sorry! Somehow I missed that. I thought that by adding the word "see" I had addressed all your comments. I'm about to go to dinner, but when I come back later I'll scroll back and figure it out. Thanks again for your very thorough reviews! |
|
@larryhastings: Please replace |
|
Thanks for all your time on this, Jelle! |
https://bugs.python.org/issue43987