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

Allow Date trait type to reject datetime instances #1429

Merged
merged 7 commits into from
Feb 4, 2021

Conversation

mdickinson
Copy link
Member

This PR makes it possible to specify that a Date trait should not accept datetime instances, which is usually the behaviour that the user expects/wants.

  • Adds an allow_datetime parameter to Date. It defaults to True, for backwards compatibility.
  • To make this possible, converts Date to a proper TraitType subclass.

Closes some but not all of #398. I'd like to leave that issue open for the question of considering changing the allow_datetime default under a deprecation warning.

Checklist

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

@mdickinson mdickinson changed the title Add allow_datetime parameter to Date trait type Allow Date trait type to reject datetime instances Feb 2, 2021
@mdickinson
Copy link
Member Author

@kitchoi Would the change in this PR meet your needs?

Copy link
Contributor

@kitchoi kitchoi left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.


# simplified signature
_OptionalDate = Optional[datetime.date]
Copy link
Contributor

Choose a reason for hiding this comment

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

The other trait types don't seem to need this Optional bit. I don't think I understand why this is needed here.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's there because None is a valid value for this trait. The other trait types (Datetime and Time) should have this, I think, else mypy will complain about the assignment of None.

Copy link
Member Author

Choose a reason for hiding this comment

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

(And arguably, None should not be a valid trait value here. That's where I'd like to get to, but we can't do it immediately because of backwards compatibility concerns.)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, it is already there for Datetime and Time: the possibility of None is there in the _BaseClass generic definition. We've switched from using _BaseInstance as a base here to something more specific (and more accurate).

@mdickinson mdickinson merged commit aece1fe into master Feb 4, 2021
@mdickinson mdickinson deleted the feature/date-disallow-datetime branch February 4, 2021 15:42
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.

None yet

2 participants