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

Support Python Enums as values for Enum trait #685

Merged
merged 7 commits into from
Jan 23, 2020
Merged

Conversation

corranwebster
Copy link
Contributor

@corranwebster corranwebster commented Dec 24, 2019

A fairly straightforward change. Given:

class FooEnum(enum.Enum):
     foo = 0
     bar = 1
     baz = 2

you can do

class Test(HasTraits):
    value = Enum(FooEnum)
    value_default = Enum(FooEnum.bar, FooEnum)

Using Python Enum with traits Enums can replace the many of the needs for the use of mapped Traits since the enums hold both a value and a name out of the box.

Edit:

More work on this, with a couple of drive-by fixes:

  • debug issue with editor; turns out to be a bug in the base EditorWithList class
  • allow sets to be used in the Enum trait as well.
  • use xgetattr instead of eval for dynamic values

@codecov-io
Copy link

codecov-io commented Dec 24, 2019

Codecov Report

Merging #685 into master will increase coverage by 0.96%.
The diff coverage is 75.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
+ Coverage   71.39%   72.35%   +0.96%     
==========================================
  Files          45       51       +6     
  Lines        6323     6352      +29     
  Branches     1286     1274      -12     
==========================================
+ Hits         4514     4596      +82     
+ Misses       1403     1361      -42     
+ Partials      406      395      -11
Impacted Files Coverage Δ
traits/util/clean_strings.py 51.72% <ø> (ø) ⬆️
traits/testing/optional_dependencies.py 94.44% <ø> (ø) ⬆️
traits/adaptation/adaptation_manager.py 98.24% <ø> (ø) ⬆️
traits/etsconfig/etsconfig.py 63.8% <ø> (+0.22%) ⬆️
traits/__init__.py 71.42% <ø> (ø) ⬆️
traits/util/import_symbol.py 100% <ø> (ø) ⬆️
traits/adaptation/adapter.py 100% <ø> (ø) ⬆️
traits/testing/unittest_tools.py 96.03% <ø> (ø) ⬆️
traits/util/async_trait_wait.py 100% <ø> (ø) ⬆️
traits/adaptation/adaptation_offer.py 94.11% <ø> (ø) ⬆️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 61672a4...b44ce97. Read the comment docs.

@corranwebster
Copy link
Contributor Author

The TraitsUI piece isn't working correctly right now.

@corranwebster
Copy link
Contributor Author

The issue with the editor is that https://github.com/enthought/traitsui/blob/master/traitsui/editor_factory.py#L317 should be listening to format_func and format_str as well, so that after init is done the mappings are all set up correctly with the final state. Otherwise whether or not you get the correct value depends on trait initialization order.

value = None
if len(values) > 0:
value = values[0]
value = collection_default(values)
Copy link
Contributor

Choose a reason for hiding this comment

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

I was trying to tackle #389 and this line is all I needed to solve #389.

Copy link
Member

Choose a reason for hiding this comment

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

@kitchoi can you clarify: if this PR gets merged, does that mean #389 can be closed? Or is there more work to do between merging this PR and solving #389?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think #389 can be closed by this PR (as of now) without more being done to it.

While #389 concerns the dictionary only, the idea is to relax the requirement on the values supporting __index__. As long as values implements __iter__, Enum can obtain the allowed values.

I understand your concern with the potential non-deterministic behaviour, e.g. for set and the dict prior to Python 3.6. But if, say, values is an instance of OrderedDict, the Enum on master will fail trying to use the first key as a default. One has to make another property traits to return a list from the keys of OrderedDict. I guess it is about how opinionated this Enum should be when it comes to setting a default value.

Copy link
Contributor

Choose a reason for hiding this comment

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

To verify:

from traits.api import HasTraits, Enum, Any


class Foo(HasTraits):

    a = Enum(values="b")

    b = Any()

    def _b_default(self):
        return {"a": 1, "b": 2}


if __name__ == "__main__":
    f = Foo()
    # f.configure_traits()
    print(repr(f.a))

On this branch, a value 'a' was printed.
On master, this results in an error:

Traceback (most recent call last):
  File "dict_enum.py", line 17, in <module>
    print(repr(f.a))
  File "/Users/kchoi/ETS/traits/traits/trait_types.py", line 2030, in _get
    value = values[0]
KeyError: 0

Copy link
Member

Choose a reason for hiding this comment

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

Hmm. I think I liked the error better. :-)

@mdickinson mdickinson added this to the 6.0.0 release milestone Jan 20, 2020
@mdickinson mdickinson self-assigned this Jan 22, 2020
@mdickinson mdickinson self-requested a review January 22, 2020 19:05
@mdickinson
Copy link
Member

I get an error running the test suite under Python 3.8:

======================================================================
ERROR: test_enum_enum (traits.tests.test_enum.EnumTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/mdickinson/Enthought/ETS/traits/traits/tests/test_enum.py", line 129, in test_enum_enum
    self.assertEqual(example.value_name, FooEnum.foo)
  File "/Users/mdickinson/Enthought/ETS/traits/traits/trait_types.py", line 2049, in _get
    if value not in values:
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.8/lib/python3.8/enum.py", line 310, in __contains__
    raise TypeError(
TypeError: unsupported operand type(s) for 'in': '_Undefined' and 'EnumMeta'

@mdickinson
Copy link
Member

Related: https://bugs.python.org/issue33217. I'm not sure why it was considered a good idea to raise TypeError rather than simply returning False.

traits/trait_base.py Outdated Show resolved Hide resolved
traits/trait_base.py Outdated Show resolved Hide resolved
Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM on a first pass.

I'm not convinced that supporting set is a good idea, but if we want to do that then there should be tests for that case, and we need to figure out what to do about getting a default value from a set: introducing non-deterministic behaviour (taking the "first" item of the set as the default) seems like a recipe for late-discovered bugs in Traits-using projects.

We need to figure out how to get the containment check to work properly on Python 3.8.

@corranwebster
Copy link
Contributor Author

I don't have an easy way to check on Python 3.8, but I have attempted to solve the value in values issue by defining a safe_contains function that does the containment check, but catches TypeError and returns False in that case. This hopefully will do something reasonable in the test case.

@mdickinson
Copy link
Member

I don't have an easy way to check on Python 3.8 [...]

Confirmed working on 3.8.

Copy link
Member

@mdickinson mdickinson left a comment

Choose a reason for hiding this comment

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

LGTM

@mdickinson mdickinson merged commit dec9ada into master Jan 23, 2020
@mdickinson mdickinson deleted the feat/enum-enums branch January 23, 2020 16:34
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