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 allowing datetime.datetime instances as values for Date traits #1441

Merged
merged 4 commits into from
Mar 15, 2021

Conversation

mdickinson
Copy link
Member

@mdickinson mdickinson commented Mar 5, 2021

This PR paves the way for eventually making it illegal to pass a datetime.datetime instance to a Date trait:

  • Change the default for allow_datetime to None rather than True
  • If allow_datetime is not supplied and the user tries to assign a datetime.datetime value to a Date trait, a DeprecationWarning is issued. In the future (ideally in Traits 7.0), a TraitError will be raised.

Rationale: right now, Date allows any value that's an instance of the Python std. lib. datetime.date class. Somewhat surprisingly, Python's datetime.datetime is a subclass of datetime.date. But in typical uses for the Date trait, we really do want to specify a date and not a datetime.

Users who do want to allow datetimes in addition to date can modify their code in a number of ways to avoid the warning: for example, using Union(Datetime(), Date()), Instance(datetime.date), or Date(allow_datetime=True).

Users can also opt in to the future behaviour right now by using Date(allow_datetime=False).

Checklist

  • Tests
  • Update API reference (docs/source/traits_api_reference) (docstring updated)
  • Update User manual (docs/source/traits_user_manual) N/A
  • Update type annotation hints in traits-stubs N/A (the default for allow_datetime has changed, but that default isn't recorded in the stub)

@@ -73,33 +73,45 @@ def test_assign_non_date(self):
message = str(exception_context.exception)
self.assertIn("must be a date or None, but", message)

def test_assign_datetime(self):
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is redundant with the new test_assign_datetime_with_allow_datetime_not_given test

traits/trait_types.py Outdated Show resolved Hide resolved
@rahulporuri rahulporuri added this to In progress in Enthought OSS Q1 2021 Mar 5, 2021
@rahulporuri rahulporuri self-requested a review March 5, 2021 14:28
@mdickinson
Copy link
Member Author

@kitchoi Would this PR meet your project's needs?

traits/trait_types.py Outdated Show resolved Hide resolved
@kitchoi
Copy link
Contributor

kitchoi commented Mar 10, 2021

@mdickinson Thank you for this. I haven't reviewed the code change in details but the intended behaviour looks good to me!

Would this PR meet your project's needs?

Recalling the other project that motivated #398, I believe the answer is yes. With the caveat that my memory of the context is rather dated, here is the details:
The deprecation warning would be rather helpful in capturing programming errors in tests, where DeprecationWarning triggered at runtime are visible. Situations where tests deviate from reality would be harder to capture, as DeprecationWarning won't be visible in production environments (e.g. web server, cronjob, GUI). Just an idea: the DeprecationWarning might need to be bumped to a FutureWarning in a later release?

For the project that revived the discussion around disallowing datetime for Date, allow_datetime=False has been preemptively installed so we won't see deprecation warnings, instead we will get an error if all due diligence has failed to capture anomalies.

Minor: Did you mean to change Union(Datetime, Date) to Union(Datetime(), Date()) in the main description too?

@mdickinson
Copy link
Member Author

Just an idea: the DeprecationWarning might need to be bumped to a FutureWarning in a later release?

Yes, we still don't have a good solution to this problem. FutureWarning isn't really the right thing: it's intended really as a warning to users of an application, rather than developers (and indeed it would result in spurious warnings that were visible to users of Traits-using applications, not just the developers of those applications).

Part of the solution might be simply to recommend that deprecation warnings always be turned on during manual testing of an application, and providing patterns for doing that. But I think this discussion is out of scope for Traits itself.

@mdickinson
Copy link
Member Author

Minor: Did you mean to change Union(Datetime, Date) to Union(Datetime(), Date()) in the main description too?

Thanks! I didn't, but it's a good idea. Done.

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

@mdickinson mdickinson merged commit 2b62cd9 into master Mar 15, 2021
Enthought OSS Q1 2021 automation moved this from In progress to Done Mar 15, 2021
@mdickinson mdickinson deleted the deprecation/datetime-values-for-date-traits branch March 15, 2021 11:34
@rahulporuri
Copy link
Contributor

I was going through the documentation to see if anything needed to be updated - but from the looks of it, Date is not documented in the "Predefined Traits" section of the docs.

We might want to include Date in the above section.

@rahulporuri rahulporuri moved this from Done to Sprint 4 : March 1 - March 12 2021 in Enthought OSS Q1 2021 Mar 15, 2021
@mdickinson
Copy link
Member Author

Agreed; opened #1445 for this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Enthought OSS Q1 2021
Sprint 4 : March 1 - March 12 2021
Development

Successfully merging this pull request may close these issues.

None yet

3 participants