Skip to content

gh-101552: Allow pydoc to display signatures in source format #124669

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 13 commits into from
Oct 9, 2024

Conversation

JelleZijlstra
Copy link
Member

@JelleZijlstra JelleZijlstra commented Sep 27, 2024

  • Add a annotations_format argument to inspect.signature, matching annotationlib.Format
  • Add an unquote_annotations argument to inspect.Signature.format, allowing string annotations to be displayed without quotes
  • Use the new functionality in pydoc to display signatures in source format.

📚 Documentation preview 📚: https://cpython-previews--124669.org.readthedocs.build/

ida = inspect_deferred_annotations
sig = inspect.Signature
par = inspect.Parameter
PORK = inspect.Parameter.POSITIONAL_OR_KEYWORD
Copy link
Member

Choose a reason for hiding this comment

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

🐷

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Nice! Some nitpicks, but nothing blocking really

@@ -838,7 +843,7 @@ function.
:class:`Signature` objects are also supported by the generic function
:func:`copy.replace`.

.. method:: format(*, max_width=None)
.. method:: format(*, max_width=None, unquote_annotations=False)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel like a totally correct use of "unquote" to me... Maybe the parameter could be called remove_annotation_quotes instead of unquote_annotation...? I think that might also reflect slightly better the fact that not all annotations will necessarily be quoted at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

We use "unquote" in various other places (e.g., https://docs.python.org/3.13/library/csv.html#csv.QUOTE_NOTNULL); I feel it's more clear and concise

Copy link
Member

Choose a reason for hiding this comment

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

"unquoted" is used in https://docs.python.org/3.13/library/csv.html#csv.QUOTE_NOTNULL as an adjective rather than a verb. I think it makes sense to talk about something being "unquoted" but less sense to talk about "unquoting" something.

Copy link
Contributor

Choose a reason for hiding this comment

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

"dequote"?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading the code correctly, if this is false (the default) we use repr on annotation values that are strings, and if it's true we skip repr only for strings. So this is a passive operation--if the value is true, we don't do something, and when it's false we do do something. I think that's why this is awkward.

I suggest the clearest way to express this is to flip it. Negate the boolean and rename it quote_annotations=True. If it's true (the default) we quote annotations, and if it's false we don't. I might even go with the wordier quote_annotation_strings=True, as we're only allowing the user to control quoting or not-quoting the annotations when they're strings. But I don't have a strong opinion about that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, changing that.

Co-authored-by: Alex Waygood <[email protected]>
@@ -722,18 +722,20 @@ function.
``from __future__ import annotations`` was used), :func:`signature` will
attempt to automatically un-stringize the annotations using
:func:`annotationlib.get_annotations`. The
*globals*, *locals*, and *eval_str* parameters are passed
*globals*, *locals*, *eval_str*, and *annotation_format* parameters are passed
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a minor thing, but: globals, locals, and eval_str are passed in to parameters with the same name when calling get_annotations. But annotation_format is passed in to a parameter with a different name (just format). It might be nice to explicitly tell that to the user.

I realize the documentation doesn't explicitly detail where these parameters go, so this would be all-new text. And when I try it in my head it always comes across as clumsy. So... should we even bother to try? Would anybody be confused if we didn't bother to explain it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I will add a separate sentence about the annotation_format parameter.

@@ -838,7 +843,7 @@ function.
:class:`Signature` objects are also supported by the generic function
:func:`copy.replace`.

.. method:: format(*, max_width=None)
.. method:: format(*, max_width=None, unquote_annotations=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm reading the code correctly, if this is false (the default) we use repr on annotation values that are strings, and if it's true we skip repr only for strings. So this is a passive operation--if the value is true, we don't do something, and when it's false we do do something. I think that's why this is awkward.

I suggest the clearest way to express this is to flip it. Negate the boolean and rename it quote_annotations=True. If it's true (the default) we quote annotations, and if it's false we don't. I might even go with the wordier quote_annotation_strings=True, as we're only allowing the user to control quoting or not-quoting the annotations when they're strings. But I don't have a strong opinion about that.

@JelleZijlstra JelleZijlstra enabled auto-merge (squash) October 9, 2024 04:39
@JelleZijlstra JelleZijlstra merged commit 7840638 into python:main Oct 9, 2024
34 checks passed
efimov-mikhail pushed a commit to efimov-mikhail/cpython that referenced this pull request Oct 9, 2024
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.

4 participants