Skip to content

Conversation

GeoffreyOnRails
Copy link

Minor documentation fix : Some methods in the cheat_sheet didn't work because of a missing import

Copy link
Collaborator

@A5rocks A5rocks left a comment

Choose a reason for hiding this comment

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

I think it might actually be better for overall documentation cohesion to not allow these to be runnable. But at the very least part of the examples is now incorrect!

@sterliakov
Copy link
Collaborator

Are you sure this is warranted? Usually you use reveal_type while you're investigating some mypy error, it doesn't stay in your code indefinitely. I don't remember exact reasons for adding reveal_type to typing altogether, but IMO it's perfectly fine for a documentation example to not import it and use the implicit typechecker definition, basically amounting to "only allowed at typechecking time, remove before running". My reading of the docs suggests that these examples are supposed to be checkable (can be fed to mypy to produce some meaningful output), not runnable by python interpreter.

@GeoffreyOnRails
Copy link
Author

Here is why it was added :

adding an implementation to typing would help document the behavior and make it more discoverable for users. Also, it means code with reveal_type() calls can run without runtime errors, useful if you want to run your tests at the same time as you're debugging a typing issue.

I don't see why it would be best NOT to import it from typing in the documentation example. Even if the reveal_type() ends up being removed, when I remove some code and imports become no longer used, I end up removing the related imports (and my IDE or static checker tools tells me to do so).

Also, it has nothing to do with mypy, but the cheat sheet is the first link the beginners in typing has been sent to by the official python doc, so I think it's best to have regular runnable python as much as we can, not just checkable, especially when it costs nothing more than adding an import.

@JelleZijlstra
Copy link
Member

When we added typing.reveal_type I was worried it would lead to people going on campaigns to add the import. I don't think it's necessary, and the way reveal_type is used it's often nicer to not need the import.

@GeoffreyOnRails
Copy link
Author

I modified the note instead, but I still believe it's not at all beginner friendly to refer to a note 100 lines lower in the cheat sheet instead of just making the code runnable...

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.

5 participants