Skip to content
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

enum error messages should use __qualname__ #78624

Closed
doerwalter opened this issue Aug 20, 2018 · 15 comments
Closed

enum error messages should use __qualname__ #78624

doerwalter opened this issue Aug 20, 2018 · 15 comments
Assignees
Labels
3.8 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@doerwalter
Copy link
Contributor

BPO 34443
Nosy @warsaw, @doerwalter, @ethanfurman, @serhiy-storchaka, @enedil, @miss-islington, @orlnub123, @danishprakash, @UnderscoreAsterisk
PRs
  • bpo-34443: return __qualname__ in enum __repr__ #8989
  • bpo-34443: Change enum __repr__s and __str__s to use the fully qualified name #9011
  • bpo-34443: Use __qualname__ instead of __name__ in enum exception messages. #14809
  • [3.7] bpo-34443: Use __qualname__ instead of __name__ in enum exception messages. (GH-14809) #19166
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/ethanfurman'
    closed_at = <Date 2020-03-25.19:51:53.479>
    created_at = <Date 2018-08-20.16:34:28.016>
    labels = ['easy', '3.8', 'type-feature', 'library']
    title = 'enum error messages should use __qualname__'
    updated_at = <Date 2020-03-25.19:54:02.508>
    user = 'https://github.com/doerwalter'

    bugs.python.org fields:

    activity = <Date 2020-03-25.19:54:02.508>
    actor = 'miss-islington'
    assignee = 'ethan.furman'
    closed = True
    closed_date = <Date 2020-03-25.19:51:53.479>
    closer = 'ethan.furman'
    components = ['Library (Lib)']
    creation = <Date 2018-08-20.16:34:28.016>
    creator = 'doerwalter'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34443
    keywords = ['patch', 'easy']
    message_count = 15.0
    messages = ['323799', '323801', '323822', '324110', '324313', '324402', '324442', '325006', '325007', '325020', '325023', '348014', '348026', '348048', '348123']
    nosy_count = 10.0
    nosy_names = ['barry', 'doerwalter', 'eli.bendersky', 'ethan.furman', 'serhiy.storchaka', 'enedil', 'miss-islington', 'orlnub123', 'danishprakash', 'underscore_asterisk']
    pr_nums = ['8989', '9011', '14809', '19166']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34443'
    versions = ['Python 3.8']

    @doerwalter
    Copy link
    Contributor Author

    The __repr__ output of an enum class should use __qualname__ instead of __name__. The following example shows the problem:

    import enum
    
    class X:
       class I:
          pass
    
    class Y:
       class I(enum.Enum):
          pass
    
    print(X.I)
    print(Y.I)

    This prints:

    <class '__main__.X.I'>
    <enum 'I'>

    I would have expected it to print

    <class '__main__.X.I'>
    <enum 'Y.I'>

    or even for maximum consistency

    <class '__main__.X.I'>
    <enum '__main__.Y.I'>

    @doerwalter doerwalter added stdlib Python modules in the Lib dir type-feature A feature request or enhancement easy labels Aug 20, 2018
    @serhiy-storchaka
    Copy link
    Member

    __qualname__ should be used only together with __module__.

    I agree that the repr of enum should be more consistent with the repr of class.

    @serhiy-storchaka serhiy-storchaka added the 3.8 only security fixes label Aug 20, 2018
    @ethanfurman ethanfurman self-assigned this Aug 20, 2018
    @UnderscoreAsterisk
    Copy link
    Mannequin

    UnderscoreAsterisk mannequin commented Aug 21, 2018

    Hi,
    I would like to work on this issue. I can submit a PR by tomorrow (or maybe even later today).

    @enedil
    Copy link
    Mannequin

    enedil mannequin commented Aug 26, 2018

    Serhiy, would it be ok to put '__module__' + '.' + __qualname__ here?

    @danishprakash
    Copy link
    Mannequin

    danishprakash mannequin commented Aug 29, 2018

    Serhiy, why should __qualname__ always be used together with __module__? I can't seem to find a valid reason, I've been through the pep.

    <made a PR due to inactivity>

    @orlnub123
    Copy link
    Mannequin

    orlnub123 mannequin commented Aug 30, 2018

    I've created a separate PR that also changes the __str__s.

    @orlnub123
    Copy link
    Mannequin

    orlnub123 mannequin commented Aug 31, 2018

    After some thinking I've come to the conclusion that making the __str__s use the fully qualified name was a bad idea. I've closed my PR.

    @serhiy-storchaka
    Copy link
    Member

    Serhiy, why should __qualname__ always be used together with __module__?

    Because what is the purpose of using __qualname__?

    @serhiy-storchaka
    Copy link
    Member

    I think using more longer name in repr and/or str for *instances* of enum classes is not good idea. They are already verbose, and this will make them more verbose.

    Actually in some cases when enum instances are exposed as module globals, I would want to make them including the module name instead of the enum class name. For example:

    >>> import socket
    >>> socket.AF_UNIX
    <AddressFamily.AF_UNIX: 1>
    >>> print(socket.AF_UNIX)
    AddressFamily.AF_UNIX

    "socket.AF_UNIX" instead of "AddressFamily.AF_UNIX" would look better to me.

    @ethanfurman
    Copy link
    Member

    Serhiy said:
    -----------

    I think using more longer name in repr and/or str for *instances* of
    enum classes is not good idea. They are already verbose, and this
    will make them more verbose.

    I'm okay with verbose reprs, as debugging is the primary feature for those (at least for me). I'm not okay with __str__ being other than what it is now (but see below).

    Serhiy also said:
    ----------------

    Actually in some cases when enum instances are exposed as module
    globals, I would want to make them including the module name instead
    of the enum class name. For example:

    >>> import socket
    >>> socket.AF_UNIX
    <AddressFamily.AF_UNIX: 1>
    >>> print(socket.AF_UNIX)
    AddressFamily.AF_UNIX

    "socket.AF_UNIX" instead of "AddressFamily.AF_UNIX" would look better
    to me.

    Since AddressFamily, and other stdlib converted constants, are created user _convert, I have no problem with that method also changing the str to be `module.member' instead.

    @ethanfurman
    Copy link
    Member

    Okay, I might be changing my mind. In most cases I suspect the difference would be minimal, but when it isn't, it really isn't. Take this example from a pydoc test:

      class Color(enum.Enum)
       |  Color(value, names=None, *, module=None, qualname=None, type=None, start=1)
       |  
       |  An enumeration.
       |  
       |  Method resolution order:
       |      Color
       |      enum.Enum
       |      builtins.object
       |  
       |  Data and other attributes defined here:
       |  
    -  |  blue = <test.test_enum.TestStdLib.Color.blue: 3>
    +  |  blue = <Color.blue: 3>
       |  
    -  |  green = <test.test_enum.TestStdLib.Color.green: 2>
    +  |  green = <Color.green: 2>
       |  
    -  |  red = <test.test_enum.TestStdLib.Color.red: 1>
    +  |  red = <Color.red: 1>

    It feels like the important information is completely lost in the noise.

    Okay, I'm rejecting the __repr__ changes. Besides the potential verbosity, there should usually only be one of any particular Enum, __module__ and __qualname__ are both readily available when there are more than one (either on accident or by design), and users can modify their own __repr__s if they like.

    I'm still thinking about the change in _convert_ to modify __str__ to use the module name instead of the class name.... Here are my questions about that:

    • Modify just __str__ or __repr__ as well?
      socket.AF_UNIX instead of AddressFamily.AF_UNIX
      <socket.AddressFamily.AF_UNIX: 1> instead of <AddressFamily.AF_UNIX: 1>

    • potential confusion that actual instances of Enum in the stdlib appear
      differently than "regular" Enums? Or perhaps call out those differences
      in the documentation as examples of customization?

    @doerwalter
    Copy link
    Contributor Author

    Can we at least get the __qualname__ in exception messages?

    Currently enum.Enum.__new__() and enum.Enum._missing_() use:

    raise ValueError("%r is not a valid %s" % (value, cls.__name__))

    IMHO this should be:

    raise ValueError("%r is not a valid %s" % (value, cls.__qualname__))

    in both spots.

    Example code:

    class Person:
        class Type(enum.Enum):
            EMPLOYEE = "employee"
            CUSTOMER = "customer"
            SUPPLIER = "supplier"
    
    class Contact:
        class Type(enum.Enum):
            EMAIL = "email"
            PHONE = "phone"
            MOBILE = "mobile"
    
    with this the following code:
    
        Person.Type('foo')

    raises the exception:

    ValueError: 'foo' is not a valid Type
    
        During handling of the above exception, another exception occurred:
    
        Traceback (most recent call last):
          File "<stdin>", line 1, in <module>
          File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 310, in __call__
            return cls.__new__(cls, value)
          File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 564, in __new__
            raise exc
          File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 548, in __new__
            result = cls._missing_(value)
          File "/usr/local/Cellar/python/3.7.4/Frameworks/Python.framework/Versions/3.7/lib/python3.7/enum.py", line 577, in _missing_
            raise ValueError("%r is not a valid %s" % (value, cls.__name__))
        ValueError: 'foo' is not a valid Type

    IMHO the exception message:

    ValueError: 'foo' is not a valid Person.Type
    

    would be much more helpful and unambiguous.

    And BTW, maybe we should suppress exception chaining here, i.e. use:

    raise ValueError("%r is not a valid %s" % (value, cls.__qualname__)) from None

    @ethanfurman
    Copy link
    Member

    Both are good suggestions. The first should definitely happen. I'll investigate the second.

    @ethanfurman ethanfurman reopened this Jul 16, 2019
    @ethanfurman
    Copy link
    Member

    If someone would like to make a PR for using __qualname__ in error messages that would be great. :)

    @ethanfurman
    Copy link
    Member

    New changeset 323842c by Ethan Furman (Walter Dörwald) in branch 'master':
    bpo-34443: Use __qualname__ instead of __name__ in enum exception messages. (GH-14809)
    323842c

    @ethanfurman ethanfurman changed the title enum repr should use __qualname__ enum error messages should use __qualname__ Mar 25, 2020
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes easy stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants