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

Check Enum.create_editor; add tests #965

Closed
mdickinson opened this issue Mar 27, 2020 · 7 comments · Fixed by #1012
Closed

Check Enum.create_editor; add tests #965

mdickinson opened this issue Mar 27, 2020 · 7 comments · Fixed by #1012
Assignees
Labels
component: test suite Issues related directly to the test suite type: enhancement
Milestone

Comments

@mdickinson
Copy link
Member

I'm suspicious of the code in Enum.create_editor, and it isn't exercised by the test suite. We should double check that it works as intended before the 6.1.0 release (and ideally, also add tests).

We should also check that the editor behaves sensibly for the various possible input types (which changed in #889).

For example: is that values = self really right? For another, there's some code that looks editor-related in the BaseEnum __init__ method, but that code never gets executed, and doesn't belong in the __init__ in the first place.

@mdickinson mdickinson added this to the 6.1.0 release milestone Mar 27, 2020
@midhun-pm
Copy link
Contributor

midhun-pm commented Mar 31, 2020

The current code on master does work for Enum's with collection arguments.
The only case I could find that caused problems was Enum([1, {1, 2}]) which errored out on calling configure_traits().
This is a valid Enum declaration where the valid values are either 1 or the set {1,2} . Is this something that we want to fix?

@mdickinson
Copy link
Member Author

That depends. What was the error message, and what was its underlying cause?

@mdickinson mdickinson added component: test suite Issues related directly to the test suite type: enhancement labels Apr 2, 2020
@midhun-pm
Copy link
Contributor

The exact cause of the bug is trying to use the set {1,2} as the key in a dict in traitsui.helper when trying to create an inverse mapping from the value to its string representation. I believe this is to update the items in the editor in case the value changes externally.

I verified that the editor works fine if the exception is caught, but this will break the automatic update on external value changes. Shall I open an issue in traitsui ?

@midhun-pm
Copy link
Contributor

However, I can't think of good common use cases where we would want an enumeration of different kind of types.

@kitchoi
Copy link
Contributor

kitchoi commented Apr 14, 2020

is that values = self really right?

This is the code that allows Enum to be passed directly to EnumFactory:
https://github.com/enthought/traitsui/blob/b7a409d4e869d1857fcf77e8be47de581e864d41/traitsui/helper.py#L91-L92

This is after a bunch of ifs fall through. I think we should just set values to self.values in Enum.create_editor.

Off-topic: When I was reviewing #988 and looking into this issue, I discovered this traitsui issue: enthought/traitsui#782

@kitchoi
Copy link
Contributor

kitchoi commented Apr 14, 2020

When I was reviewing #988 and looking into this issue, I discovered this traitsui issue: enthought/traitsui#782

enthought/traitsui#782 is not entirely orthogonal: because if we want the Enum.create_editor to create an editor that behaves correctly, then we would have to workaround enthought/traitsui#782 by assigning the value after the editor is instantiated:

        editor = EnumEditor(
            name=name,
            cols=self.cols or 3,
            evaluate=self.evaluate,
            format_func=self.format_func,
            mode=self.mode if self.mode else "radio",
        )
        if self.name is None:
            # workaround enthought/traitsui#782
            editor.values = values
        return editor

I'd think this format_func and evaluate should not be passed via traits.Enum... but that's a separate discussion.

@mdickinson
Copy link
Member Author

Closed by #988.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: test suite Issues related directly to the test suite type: enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants