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

Value-changing getters on dynamic Trait types are problematic #1390

Open
mdickinson opened this issue Jan 13, 2021 · 4 comments
Open

Value-changing getters on dynamic Trait types are problematic #1390

mdickinson opened this issue Jan 13, 2021 · 4 comments

Comments

@mdickinson
Copy link
Member

The Enum and Range trait types have trait getters which can report a different value from the one stored.

One way in which this is problematic (there are others) is that if those Trait types are inside another container, the get magic is no longer invoked.

Example to follow shortly.

@mdickinson
Copy link
Member Author

Here's an example of the problematic behaviour: our dynamic Enum value is modified at get time for a single instance, but not for an Enum inside a List:

from traits.api import Enum, HasStrictTraits, List, Str

class OrderRecord(HasStrictTraits):

    meal_choices = List(Str)

    single_customer_order = Enum(values="meal_choices")

    group_order = List(Enum(values="meal_choices"))


# Works as expected: order is modified when the menu changes.
record = OrderRecord(meal_choices=["curry", "cornflakes", "carrots"])
record.single_customer_order = "curry"
# "Sorry Sir, the curry is off. Perhaps you'd like the cornflakes instead?"
record.meal_choices.remove("curry")
assert record.single_customer_order == "cornflakes"  # passes, as expected

# Fails: the list isn't modified when the menu changes.
record = OrderRecord(meal_choices=["curry", "cornflakes", "carrots"])
record.group_order = ["carrots", "curry"]
record.meal_choices.remove("curry")
assert record.group_order == ["carrots", "cornflakes"]  # unexpected failure

@mdickinson
Copy link
Member Author

In theory, we could rework the getters for Lists, Tuples, etc. to invoke the getters for individual traits. In practice, that's going to be a lot of work, and it's really not clear that it's the right thing to do. An alternative may be to deprecate the value-changing behaviour, or at least warn about it in the documentation and note that it's not wise to depend on it.

@mdickinson
Copy link
Member Author

mdickinson commented Jan 13, 2021

One way in which this is problematic (there are others)

To elaborate on this: the main problem with this get-time value changing is that the apparent value of the trait will have changed, but observers will not receive any notification about the value change.

An additional issue is the surprising non-stickiness of the change: in the original example, if I add "curry" back into the meal choices, then record.single_customer_order reverts from "cornflakes" to "curry".

@mdickinson
Copy link
Member Author

A few points from discussion in another channel:

To summarize the issue: we have a trait where the validity of the value is constrained by the value of other traits.

  • In the case where the constraints don't change (e.g., a Range where the min and max stay fixed, or an Enum where the collection of valid values stays fixed for the lifetime of the object), there's no real issue here
  • Note that even in the above case, there's still value in the constraints being dynamic: the dynamic constraints allow different objects of the same type to have different constraints, even if those constraints aren't changed during the lifetime of any given object.
  • The problems occur when the values of the constraining traits change.
  • One possible "solution" to the particular bug identified in the first post (that the trait behaves differently inside a collection) would be to have the 'get' operation on a List or Tuple trait actually go and do a get on each individual element of the collection. While this seems logical, it would be a major change with major implications, and we almost certainly don't want to go this way.

Part of the "solution" here would likely be to provide recommended patterns for use-cases where the constraints do need to change dynamically. Those patterns could be in the form of documentation, or something closer to re-usable code. A likely pattern for the solution would involve trait listeners on the constraints that reset the constrained value appropriately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant