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

Add DefaultValue.disallow #1546

Merged
merged 8 commits into from
Sep 24, 2021
Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Sep 21, 2021

This PR adds a new value to the DefaultValue enumeration: DefaultValue.disallow. The intent is to use this for trait types that don't have a valid default. Right now that's Event and Disallow, but it could potentially be extended to other trait types in the future. The main feature of DefaultValue.disallow is that an attempt to get the default value of the corresponding CTrait instance using default_value_for will fail, raising ValueError.

Rationale: With the current situation, the Event trait has no way to communicate that its default shouldn't be used: the default_value_for method succeeds, and indicates that the default value is a constant, and that that constant value Undefined. That means that a trait like Tuple(Event(), Event()) has a default value of (Undefined, Undefined). We could potentially update the logic in traits to check for Undefined everywhere and interpret that as "no default", but it seems cleaner to introduce a new default value type for this.

This PR doesn't fix #1541, but it should enable an easy fix for that issue.

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference)
  • [ ] Update User manual (docs/source/traits_user_manual) N/A
  • Update type annotation hints in traits-stubs

Update: we renamed from DefaultValue.unsupported to DefaultValue.disallow; I've edited the description and title accordingly.

traits/constants.py Outdated Show resolved Hide resolved
@@ -215,4 +219,5 @@ class DefaultValue(IntEnum):
DefaultValue.callable_and_args: "factory",
DefaultValue.callable: "method",
DefaultValue.trait_set_object: "set",
DefaultValue.unsupported: "unsupported"
Copy link
Member Author

Choose a reason for hiding this comment

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

The default_kind property is essentially unused; I'm including this only for completeness.

@mdickinson
Copy link
Member Author

There's also potential to use this in the future for Instance (or other) traits where we want to enforce that the default of None is never used in practice.

@mdickinson
Copy link
Member Author

I'm not wedded to the "unsupported" name here; alternative naming suggestions welcome. I was also considering "disallow", to match the Disallow trait type.

@mdickinson
Copy link
Member Author

Related: the CantHaveDefaultValue constant:

traits/traits/has_traits.py

Lines 110 to 111 in eb62eb6

# Trait types which cannot have default values
CantHaveDefaultValue = ("event", "delegate", "constant")

Copy link
Contributor

@rahulporuri rahulporuri left a comment

Choose a reason for hiding this comment

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

IIRC from the internal discussion, we decided to rename this to disallowed instead of unsupported.

@mdickinson
Copy link
Member Author

IIRC from the internal discussion, we decided to rename this to disallowed instead of unsupported.

Yes, I'll rename (but I think to "disallow" rather than "disallowed").

…e-type' into feature/unsupported-default-value-type
@mdickinson mdickinson changed the title Add DefaultValue.unsupported Add DefaultValue.disallow Sep 23, 2021
@mdickinson
Copy link
Member Author

Updated for the suggested renaming; I've also edited the PR title and description to match to make it easier to find in the future.

traits/ctraits.c Outdated Show resolved Hide resolved
traits/constants.py Show resolved Hide resolved
traits/tests/test_trait_types.py Show resolved Hide resolved
@mdickinson mdickinson merged commit 82658dc into main Sep 24, 2021
@mdickinson mdickinson deleted the feature/unsupported-default-value-type branch September 24, 2021 07:03
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.

Reading from an Event trait inside a Tuple trait should be an error
3 participants