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

Fix allow_none to work correctly on BaseInstance objects #1433

Merged
merged 8 commits into from
Feb 9, 2021

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Feb 4, 2021

Currently instances of BaseInstance don't clone themselves correctly when called with a new version of allow_none. This means that Time(allow_none=False) and Datetime(allow_none=False) don't work as expected:

Example (on the master branch):

>>> from traits.api import *
>>> Slice = BaseInstance(slice)
>>> SliceNotNone = Slice(allow_none=False)
>>> Slice.validate(None, "dummy", None)  # accepts None, as expected
>>> SliceNotNone.validate(None, "dummy", None)  # accepts None, but shouldn't

This PR fixes the issue by extending the base class clone method to add special handling of allow_none. With the PR, the above example becomes:

>>> from traits.api import *
>>> Slice = BaseInstance(slice)
>>> SliceNotNone = Slice(allow_none=False)
>>> Slice.validate(None, "dummy", None)  # accepts None, as expected
>>> SliceNotNone.validate(None, "dummy", None)  # rejects None, as expected  
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/mdickinson/Enthought/ETS/traits/traits/trait_types.py", line 3361, in validate
    self.validate_failed(object, name, value)
  File "/Users/mdickinson/Enthought/ETS/traits/traits/trait_types.py", line 3211, in validate_failed
    self.error(object, name, value)
  File "/Users/mdickinson/Enthought/ETS/traits/traits/base_trait_handler.py", line 75, in error
    object, name, self.full_info(object, name, value), value
traits.trait_errors.TraitError: The 'dummy' trait must be a slice, but a value of None <class 'NoneType'> was specified.

We also take the opportunity to add some basic tests for the Datetime and Time trait types.

Fixes #495

Checklist

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

@mdickinson
Copy link
Member Author

Note: while this is a bugfix, it's a long-standing bug that hasn't caused major issues, so it doesn't seem worth backporting to any bugfix release on the 6.2.x branch.

@@ -281,6 +281,11 @@ def clone(self, default_value=NoDefaultSpecified, **metadata):
A dictionary of metadata names and corresponding values as
arbitrary keyword arguments.

Returns
Copy link
Member Author

Choose a reason for hiding this comment

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

Drive-by documentation fix; no associated change in behaviour.

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.

LGTM with one question - I'm not sure why this fix only works on BaseInstance objects and not on BaseInstance subclasses? Won't the subclasses inherit the behavior via the inherited clone method?

traits/trait_types.py Outdated Show resolved Hide resolved
@mdickinson
Copy link
Member Author

mdickinson commented Feb 5, 2021

I'm not sure why this fix only works on BaseInstance objects and not on BaseInstance subclasses?

It works on both. I'll fix the title to be less misleading. (The point of the title is that the usual pattern is some_trait_class(allow_none=True), where allow_none is a parameter to the initializer of the TraitType subclass, but the bug we're fixing is for the case of trait_instance(allow_none=True), where trait_instance is already an instance of BaseInstance.)

@mdickinson mdickinson changed the title Fix allow_none to work correctly on BaseInstance objects (not subclasses!) Fix allow_none to work correctly on BaseInstance objects Feb 5, 2021
@mdickinson
Copy link
Member Author

mdickinson commented Feb 5, 2021

To be clear: before this PR

  • If SomeTraitType is a subclass of BaseInstance, then foo = SomeTraitType(allow_none=False) already works as expected
  • If SomeThing is an instance of BaseInstance (as e.g., Datetime and Time are), then foo = SomeThing(allow_none=False) does not work as intended - None is still allowed

This PR fixes the second part above.

@mdickinson mdickinson merged commit a297ee6 into master Feb 9, 2021
@mdickinson mdickinson deleted the fix/time-allow-none branch February 9, 2021 12:25
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.

allow_none given to Date and Time are not respected
2 participants