X Tutup
Skip to content

bpo-43987: Add "Annotations Best Practices" HOWTO doc.#25746

Merged
larryhastings merged 11 commits intopython:masterfrom
larryhastings:annotations_best_practices
May 2, 2021
Merged

bpo-43987: Add "Annotations Best Practices" HOWTO doc.#25746
larryhastings merged 11 commits intopython:masterfrom
larryhastings:annotations_best_practices

Conversation

@larryhastings
Copy link
Copy Markdown
Contributor

@larryhastings larryhastings commented Apr 30, 2021

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 the typing module.

Comment on lines +79 to +80
a:int = 3
b:str = 'abc'
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Comment on lines +124 to +126
for you. Un-stringizing is a complicated process, and
it's best to let :func:`inspect.get_annotations` do it
for you.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra Apr 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 | str and from __future__ import annotations before 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@larryhastings
Copy link
Copy Markdown
Contributor Author

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, __annotations__ isn't like other dunder attributes, so maybe its asymmetry is inevitable.)

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
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might work better as a bulleted list, instead of a long string of semicolon-separated fragments.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this looks a lot more readable! Left a few tiny suggestions.

@larryhastings
Copy link
Copy Markdown
Contributor Author

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!
@larryhastings
Copy link
Copy Markdown
Contributor Author

I realize it seems like an unnecessary formality at this point, but I would still prefer (and enjoy!) a LGTM review here.

@JelleZijlstra
Copy link
Copy Markdown
Member

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

@larryhastings
Copy link
Copy Markdown
Contributor Author

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!

Copy link
Copy Markdown
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@larryhastings larryhastings merged commit 49b26fa into python:master May 2, 2021
@bedevere-bot
Copy link
Copy Markdown

@larryhastings: Please replace # with GH- in the commit message next time. Thanks!

@larryhastings larryhastings deleted the annotations_best_practices branch May 2, 2021 04:19
@larryhastings
Copy link
Copy Markdown
Contributor Author

Thanks for all your time on this, Jelle!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs Documentation in the Doc dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

X Tutup