Skip to content

bpo-32769: Write annotation entry for glossary #6657

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 8 commits into from
May 14, 2018

Conversation

andresdelfino
Copy link
Contributor

@andresdelfino andresdelfino commented Apr 30, 2018

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.

Thanks, looks good! I have just few minor comments.

Doc/glossary.rst Outdated
An :term:`annotation` of a function or method.

For example, this function is annotated as requiring its parameters to be
int and as returning an int as well::
Copy link
Member

Choose a reason for hiding this comment

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

Make "int" text here a reference to builtin int class here (appears twice in this sentence), I think you can use :class: for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ Done

Doc/glossary.rst Outdated
:func:`typing.get_type_hints`.
An :term:`annotation` of a global variable, or class variable.

For example, this variable is annotated as an int::
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about making "int" a reference here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ Done

Doc/glossary.rst Outdated

For example, this variable is annotated as an int::

count: int = 0
Copy link
Member

Choose a reason for hiding this comment

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

Maybe show also an example where there is no right hand side?

class C:
    field: int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ Done

Doc/glossary.rst Outdated
function object.
An :term:`annotation` of a function or method.

For example, this function is annotated as requiring its parameters to be
Copy link
Member

Choose a reason for hiding this comment

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

"requiring" is a too strong word, I would replace with "expecting".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ Done

Doc/glossary.rst Outdated
An :term:`annotation` of a function or method.

For example, this function is annotated as requiring its parameters to be
int and as returning an int as well::
Copy link
Member

Choose a reason for hiding this comment

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

"int" -> "an int" at beginning of this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't use "an int" there because we are talking about two parameters, in plural. Anyway, I changed this lines a little, I hope it makes it clearer.

Doc/glossary.rst Outdated
:func:`typing.get_type_hints`.
An :term:`annotation` of a global variable, or class variable.

For example, this variable is annotated as an int::
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 reformulate it as this variable is annotated as taking :class:int values or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ Done

Doc/glossary.rst Outdated

count: int = 0

When annotating variables, 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.

Double colon here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️ Done

Doc/glossary.rst Outdated
attribute of a class or module object and can be accessed using
:func:`typing.get_type_hints`.
An :term:`annotation` of a global variable, or class variable.

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 this empty line is not needed

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 think its best to separate a concise description of the term from the example.

If you still think it's best to put them in the same paragraph, I'll make it so :)

@andresdelfino
Copy link
Contributor Author

@ilevkivskyi should I keep "class attributes" to refer to instance variables, or should I use the later to separate between data attributes (instance variables) and behavior attributes (methods)?

I think "class attributes" is somewhat ambiguous.

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.

I like how this looks now. Thanks!

@ilevkivskyi ilevkivskyi merged commit f2290fb into python:master May 14, 2018
@andresdelfino andresdelfino deleted the annotations-glosary-entry branch May 14, 2018 19:06
andresdelfino added a commit to andresdelfino/cpython that referenced this pull request May 14, 2018
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.

4 participants