Skip to content

Conversation

@andresdelfino
Copy link
Contributor

@andresdelfino andresdelfino commented May 14, 2018

@andresdelfino
Copy link
Contributor Author

@ilevkivskyi I would really appreciate any input, when you have some spare time of course.

Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Sorry, I lost track a bit, here are few superficial comments. I would rather ask @gvanrossum to take a look at wording here (if he has bandwidth of course).

Doc/glossary.rst Outdated
A metadata value associated with a global variable, a class attribute or a
function or method parameter or return value, that stores a
:term:`type hint`.
A metadata value associated with a variable, a class attribute or a
Copy link
Member

Choose a reason for hiding this comment

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

I may guess "value" is what @gvanrossum didn't like, and I agree with him.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I guess this will be the most difficult part to get right 😅

Doc/glossary.rst Outdated
Annotating local variables will cause the interpreter to always make them
locals::

def f():
Copy link
Member

Choose a reason for hiding this comment

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

I think there are too much details here, it is glossary, not a spec.

Doc/glossary.rst Outdated
def broadcast_message(
message: str,
servers: List[Tuple[Tuple[str, int], Dict[str, str]]]) -> None:
...
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 also make this example shorter.

Doc/glossary.rst Outdated
Type aliases are useful for simplifying complex
:term:`type hints <type hint>`. For example ::
Type aliases are useful for simplifying :term:`type hints <type hint>`.
For example ::
Copy link
Member

Choose a reason for hiding this comment

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

Honestly this example would look better if it read "For example [bad code] could be made more readable like this [good code]" (currently it shows [good code] first).

Doc/glossary.rst Outdated
:func:`bytes.splitlines` for an additional use.

variable annotation
An :term:`annotation` of a variable, or a class attribute.
Copy link
Member

Choose a reason for hiding this comment

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

This should not have a comma.

Doc/glossary.rst Outdated
A metadata value associated with a global variable, a class attribute or a
function or method parameter or return value, that stores a
:term:`type hint`.
A label with no defined semantics associated with a variable, a class
Copy link
Member

Choose a reason for hiding this comment

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

I think "no defined semantics" is too strong -- IMO it should say that by convention annotations are usually used for type hints (i.e. as type annotations).

Also I'm not too excited about the word "label" here but I agree it's better than "metadata value".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about "syntactic construct"? Would that be too wordy?

Doc/glossary.rst Outdated

Class variables can be specified as such through
Variables can be specified as expected to be class variables through
:term:`type hints <type hint>`.
Copy link
Member

Choose a reason for hiding this comment

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

If you're going to be this specific I recommend calling out ClassVar by name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree that this was getting too specific. I left the class variable definition, though, because I think it's useful. Let me know if you think it is not and I'll remove it.

Doc/glossary.rst Outdated
For example, this function has its parameters annotated as taking
:class:`int` arguments and its return value annotated as being an
:class:`int` as well::
Function annotations can be
Copy link
Member

Choose a reason for hiding this comment

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

can be -> are usually used for

Doc/glossary.rst Outdated
A synonym for a type, created by assigning the type to an identifier.

Type aliases are useful for simplifying :term:`type hints <type hint>`.
For example ::
Copy link
Member

Choose a reason for hiding this comment

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

I'd write the bad example first and the improved example next, with words such as "For example:: [bad example] can be rewritten with more clarity as:: [good example]"

Doc/glossary.rst Outdated
A specification about the expected type for a global variable, class
variable, function or method parameter or return value.
An :term:`annotation` that specifies the expected type for a variable, class
attribute, function or method parameter or return value.
Copy link
Member

Choose a reason for hiding this comment

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

Rewrite as "[...] for a variable, a class attribute, or a function parameter or return value." (The repetition of "a" makes it clear that the last "or" binds more tightly than the other.)

Doc/glossary.rst Outdated
completion and refactoring.

Type hints are stored in :term:`annotations <annotation>`.
Type hints of global variables, class attributes, functions and methods
Copy link
Member

Choose a reason for hiding this comment

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

functions and methods -> function

Doc/glossary.rst Outdated
field: 'annotation'

When annotating variables, assignment is optional::
Variable annotations can be
Copy link
Member

Choose a reason for hiding this comment

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

can be -> are usually used for

Doc/glossary.rst Outdated
An :term:`annotation` of a variable, or a class attribute.

For example, this variable is annotated as taking :class:`int` values::
When annotating variables or class attributes, assignment is optional ::
Copy link
Member

Choose a reason for hiding this comment

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

I'd say "a variable or a class attribute"

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

ALmost there.

Doc/glossary.rst Outdated
Annotations can be :term:`type hints <type hint>`.

See :pep:`484` and :pep:`526`, which describe this functionality.
See :term:`variable annotation`, :term:`function annotation`, :pep:`484` and :pep:`526`, which describe this functionality.
Copy link
Member

Choose a reason for hiding this comment

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

Please break this long line.

Doc/glossary.rst Outdated
:term:`type hints <type hint>`: for example this function is expected to take two
:class:`int` arguments and is also expected to have an :class:`int`
return value::
return value ::
Copy link
Member

Choose a reason for hiding this comment

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

Why is there a space before the :: ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought it was custom 😅

Doc/glossary.rst Outdated
Type hints of global variables, class attributes, function or method
parameter or return value, but not local variables, can be accessed using
Type hints of global variables, class attributes, and functions
, but not local variables, can be accessed using
Copy link
Member

Choose a reason for hiding this comment

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

Move comma to preceding line.

Doc/glossary.rst Outdated
function or method parameter or return value, with no defined semantics.
A label associated with a variable, a class
attribute or a function parameter or return value,
used, by convention, for :term:`type hints <type hint>`.
Copy link
Member

Choose a reason for hiding this comment

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

there's no need for commas around "by convention".

@ilevkivskyi ilevkivskyi merged commit 6e33f81 into python:master May 26, 2018
@miss-islington
Copy link
Contributor

Thanks @andresdelfino for the PR, and @ilevkivskyi for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Sorry, @andresdelfino and @ilevkivskyi, I could not cleanly backport this to 3.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6e33f810c9e3a549c9379f24cf1d1752c29195f0 3.7

@miss-islington
Copy link
Contributor

Sorry, @andresdelfino and @ilevkivskyi, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 6e33f810c9e3a549c9379f24cf1d1752c29195f0 3.6

@ilevkivskyi
Copy link
Member

@andresdelfino Could you please make manual backports of these changes? (Make PRs against 3.6 and 3.7 branches).

@andresdelfino
Copy link
Contributor Author

Yes, I was working on them already :)

@andresdelfino andresdelfino deleted the type-hints branch May 26, 2018 12:50
andresdelfino added a commit to andresdelfino/cpython that referenced this pull request May 26, 2018
…ies (pythonGH-6829).

(cherry picked from commit 6e33f81)

Co-authored-by: Andrés Delfino <[email protected]>
@bedevere-bot
Copy link

GH-7127 is a backport of this pull request to the 3.7 branch.

andresdelfino added a commit to andresdelfino/cpython that referenced this pull request May 26, 2018
…ies (pythonGH-6829).

(cherry picked from commit 6e33f81)

Co-authored-by: Andrés Delfino <[email protected]>
@bedevere-bot
Copy link

GH-7128 is a backport of this pull request to the 3.6 branch.

gvanrossum pushed a commit that referenced this pull request May 26, 2018
…ies (GH-6829) (#7127)

(cherry picked from commit 6e33f81)

Co-authored-by: Andrés Delfino <[email protected]>
ilevkivskyi pushed a commit that referenced this pull request May 26, 2018
…ies (GH-6829) (GH-7128)

* [3.6] bpo-32769: A new take on annotations/type hinting glossary entries (GH-6829).
(cherry picked from commit 6e33f81)

Co-authored-by: Andrés Delfino <[email protected]>

* Typo fix spotted by Guido
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants