Skip to content

bpo-29974: Change typing.TYPE_CHECKING doc example #982

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 26, 2017

Conversation

Mortal
Copy link
Contributor

@Mortal Mortal commented Apr 3, 2017

The documentation of typing.TYPE_CHECKING has an example (introduced in issue #26141) that would lead to NameError at runtime. The example shows how to limit the import of "expensive_mod" to type checkers, but then goes on to use "expensive_mod.some_type" in a type annotation that is evaluated at runtime ("local_var: expensive_mod.some_type"). The use case of TYPE_CHECKING is probably meant for type annotations placed in comments, e.g. "local_var # type: expensive_mod.some_type".

@mention-bot
Copy link

@Mortal, thanks for your PR! By analyzing the history of the files in this pull request, we identified @berkerpeksag, @zware and @brettcannon to be potential reviewers.

@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately our records indicate you have not signed the CLA. For legal reasons we need you to sign this before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

Copy link
Member

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

Looks good to me, but I'm not really a typing guru so I'm just pinging the author of the original commit. @ilevkivskyi could you confirm that the proposed patch is correct?

@ilevkivskyi
Copy link
Member

The original example is correct, annotations for local variables (i.e. in function scope) are not evaluated at runtime. The motivation for this was to reduce the possible runtime overhead of using annotations, but it also helps with forward references/import cycles.

I think this example should stay. Maybe we could extend it with a function parameter annotation, to show that types should be string literals in the places where they are actually evaluated. For example something like this:

if TYPE_CHECKING:
    import expensive_mod
 
def fun(arg: 'expensive_mod.some_type') -> None:
    local_var: expensive_mod.another_type = other_fun()

@ilevkivskyi
Copy link
Member

Also it is probably worth fixing PEP 8 here and use SomeType instead of some_type.

@Mortal
Copy link
Contributor Author

Mortal commented Apr 4, 2017

@ilevkivskyi Thanks for the explanation -- I did not know that type annotations on local variables in function scope are not evaluated. I had naively tested with a type annotation on a variable in module-scope. (I guess it makes sense to ensure that a type annotation is evaluated at most once.)

I will update the patch with @ilevkivskyi's proposed changes.

@Mortal Mortal changed the title bpo-29974: typing.TYPE_CHECKING doc example is incorrect Change typing.TYPE_CHECKING doc example Apr 4, 2017
@Mortal
Copy link
Contributor Author

Mortal commented Apr 4, 2017

@berkerpeksag It seems GitHub thinks your approval of the old patch still applies even though I've rewritten the entire patch. Is my addition of a short recap of PEP 484/526 OK?

def fun(arg: 'expensive_mod.SomeType') -> None:
local_var: expensive_mod.AnotherType = other_fun()

(Note that the first type annotation must be enclosed in quotes, making it a
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: Parentheses are not needed.


(Note that the first type annotation must be enclosed in quotes, making it a
"forward reference", to hide the expensive_mod reference from the
interpreter runtime. Type annotations for local variables are not evaluated,
Copy link
Member

Choose a reason for hiding this comment

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

Please add two spaces after a full stop. See http://cpython-devguide.readthedocs.io/documenting.html#use-of-whitespace for details.

@Mariatta Mariatta changed the title Change typing.TYPE_CHECKING doc example bpo-29974: Change typing.TYPE_CHECKING doc example Apr 10, 2017
@Mariatta
Copy link
Member

Please keep the issue number in the PR description. I added bpo-29974: back to the PR title.

@Mortal
Copy link
Contributor Author

Mortal commented Apr 20, 2017

I've updated the docs according to review from @berkerpeksag.

Sorry about removing the issue number in the PR title.

local_var: expensive_mod.AnotherType = other_fun()

Note that the first type annotation must be enclosed in quotes, making it a
"forward reference", to hide the expensive_mod reference from the
Copy link
Member

Choose a reason for hiding this comment

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

I would propose to use back-quotes for expensive_mod here. Like this:

``expensive_mod``

Otherwise LGTM.

* Fix PEP 8 (SomeType instead of some_type)
* Add a function parameter annotation
* Explain, using wording from PEP 484 and PEP 526,
  why one annotation is in quotes and another is not.

Suggested by Ivan Levkevskyi.
@Mortal
Copy link
Contributor Author

Mortal commented Apr 20, 2017

Good catch about using backquotes. I've updated according to review from @ilevkivskyi and squashed to a single commit.

@berkerpeksag berkerpeksag merged commit 87c07fe into python:master Apr 26, 2017
@berkerpeksag
Copy link
Member

Thanks!

berkerpeksag pushed a commit to berkerpeksag/cpython that referenced this pull request Apr 26, 2017
* Fix PEP 8 (SomeType instead of some_type)
* Add a function parameter annotation
* Explain, using wording from PEP 484 and PEP 526,
  why one annotation is in quotes and another is not.

Suggested by Ivan Levkevskyi.

(cherry picked from commit 87c07fe)
berkerpeksag pushed a commit to berkerpeksag/cpython that referenced this pull request Apr 26, 2017
* Fix PEP 8 (SomeType instead of some_type)
* Add a function parameter annotation
* Explain, using wording from PEP 484 and PEP 526,
  why one annotation is in quotes and another is not.

Suggested by Ivan Levkevskyi.

(cherry picked from commit 87c07fe)
berkerpeksag added a commit that referenced this pull request Apr 26, 2017
* Fix PEP 8 (SomeType instead of some_type)
* Add a function parameter annotation
* Explain, using wording from PEP 484 and PEP 526,
  why one annotation is in quotes and another is not.

Suggested by Ivan Levkevskyi.

(cherry picked from commit 87c07fe)
berkerpeksag added a commit that referenced this pull request Apr 26, 2017
* Fix PEP 8 (SomeType instead of some_type)
* Add a function parameter annotation
* Explain, using wording from PEP 484 and PEP 526,
  why one annotation is in quotes and another is not.

Suggested by Ivan Levkevskyi.

(cherry picked from commit 87c07fe)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants