Skip to content

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

Closed

Conversation

larryhastings
Copy link
Contributor

@larryhastings larryhastings commented Apr 27, 2023


📚 Documentation preview 📚: https://pep-previews--3127.org.readthedocs.build/

@larryhastings larryhastings changed the title Change the defined format values to an IntEnum. PEP 649: Change the defined format values to an IntEnum. Apr 27, 2023
Copy link
Member

@AA-Turner AA-Turner left a 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``
Copy link
Member

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?

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'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!

Comment on lines +734 to +735
FORMAT_MIN = AnnotationFormat.VALUE
FORMAT_MAX = AnnotationFormat.SOURCE
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 718 to +725
``inspect`` module::

```
class AnnotationFormat(enum.IntEnum):
VALUE = 1
FORWARDREF = 2
SOURCE = 3
```
Copy link
Member

@AA-Turner AA-Turner Apr 27, 2023

Choose a reason for hiding this comment

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

Syntax fix:

Suggested change
``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

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 (in other PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AA-Turner AA-Turner changed the title PEP 649: Change the defined format values to an IntEnum. PEP 649: Change the defined format values to an IntEnum Apr 27, 2023
@larryhastings
Copy link
Contributor Author

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.

@AA-Turner
Copy link
Member

So long as #3138 contains all the changes you wish to make I think it makes sense to close this PR.

A

@larryhastings
Copy link
Contributor Author

Okay, let's close this one and move the work to #3138.

@larryhastings larryhastings deleted the six_forty_nine_intenum branch May 8, 2023 22:32
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.

3 participants