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

Deprecate None values for Date, Datetime and Time trait types #1444

Merged

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Mar 5, 2021

This PR builds on #1441 to also deprecate acceptance of None values for Date traits. While I think this is the right way to go, this does introduce a discrepancy with the Datetime and Time classes, where None is still accepted by default with no deprecation warning issued.

Update: the PR has been extended to cover Datetime and Time classes; this required realising the Datetime and Time trait types (so that they're no longer instances of BaseInstance).

Closes #1438

Checklist

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

@mdickinson mdickinson changed the title Deprecation/datetime and none values for date traits Deprecate None values for Date, Datetime and Time trait types Mar 5, 2021
@@ -44,7 +44,7 @@ class HasTimeTraits(HasStrictTraits):
none_allowed = Time(allow_none=True)


class TestDatetime(unittest.TestCase):
class TestTime(unittest.TestCase):
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 copypasta fix, unrelated to the main purpose of the PR

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 minor question. We already have the issue regd documenting Date, Datetime and Time so there's no documentation changes that need to be made in this PR.

Just to confirm, can there be any issues rewriting the traits as subclasses of TraitType instead of using BaseInstance?

Finally, how do we plan to track the work needed for traits 7.0.0? Do we want to create an issue now/a milestone?

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

Just to confirm, can there be any issues rewriting the traits as subclasses of TraitType instead of using BaseInstance?

Yes. There are potential behaviour changes here, but mostly in a good direction: that is, treating an instance of TraitType as though it were a subclass of TraitType is fraught with issues.

For example, if anyone is "instantiating" Datetime or Time in their code and making use of the particular keyword arguments that BaseInstance understands, their code will change behaviour.

But I checked existing code (to the extent that that code was available to me) for usage patterns, and I would expect the vast majority of existing code to be unaffected by this change (beyond the intentional part of the change - the new DeprecationWarning).

@mdickinson
Copy link
Member Author

Finally, how do we plan to track the work needed for traits 7.0.0? Do we want to create an issue now/a milestone?

The milestone already exists, and has done for some time. I already have an issue in that milestone as a reminder to review deprecations.

@mdickinson
Copy link
Member Author

I already have an issue in that milestone as a reminder to review deprecations.

#1268

@mdickinson mdickinson merged commit 5f2effc into master Mar 19, 2021
@mdickinson mdickinson deleted the deprecation/datetime-and-none-values-for-date-traits branch March 19, 2021 10:52
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.

Change default for allow_none and allow_datetime on Date and related Traits
2 participants