-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
PEP 649: Change the defined format values to an IntEnum #3127
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One syntax error to fix, two other questions.
A
@@ -72,7 +72,8 @@ The functionality is accessed via a new keyword-only parameter, | |||
``format``. ``format`` allows the user to request | |||
the annotations from these functions | |||
in a specific format. | |||
Format identifiers are always predefined integer values. | |||
Format identifiers are defined in the :mod:`inspect` module | |||
in a new ``AnnotationFormat`` enum, which is an ``enum.IntEnum``. | |||
The formats defined by this PEP are: | |||
|
|||
* ``inspect.VALUE = 1`` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this list also need updating?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you have in mind. The enum values are consistent between the two definitions, they're not out-of-date.
I'm gonna consider this "done", as in, there was no problem. If you still have feedback on this, please add it to the other PR, #3138. Thanks!
FORMAT_MIN = AnnotationFormat.VALUE | ||
FORMAT_MAX = AnnotationFormat.SOURCE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If now an enum, do these still make sense, as one could just iterate through the enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also don't see a strong use case for these, so maybe we should just drop them. We can always re-add them in a later version if a good use case comes up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed. Iterating through the enum is an even better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(work is in other PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
``inspect`` module:: | ||
|
||
``` | ||
class AnnotationFormat(enum.IntEnum): | ||
VALUE = 1 | ||
FORWARDREF = 2 | ||
SOURCE = 3 | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax fix:
``inspect`` module:: | |
``` | |
class AnnotationFormat(enum.IntEnum): | |
VALUE = 1 | |
FORWARDREF = 2 | |
SOURCE = 3 | |
``` | |
``inspect`` module: | |
.. code:: python | |
class AnnotationFormat(enum.IntEnum): | |
VALUE = 1 | |
FORWARDREF = 2 | |
SOURCE = 3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done (in other PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uh, sorry folks. I forgot I created this PR last week, and re-did the work in a new PR. Should I remove the change from that PR and keep this one? Should we give up on this PR and move the conversation over there? I'm game to fix it, however best to do that. |
So long as #3138 contains all the changes you wish to make I think it makes sense to close this PR. A |
Okay, let's close this one and move the work to #3138. |
📚 Documentation preview 📚: https://pep-previews--3127.org.readthedocs.build/