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

Date and datetime #348

Merged
merged 1 commit into from
Jul 31, 2019
Merged

Date and datetime #348

merged 1 commit into from
Jul 31, 2019

Conversation

randomstuff
Copy link
Contributor

@randomstuff randomstuff commented Jul 5, 2019

Rename Date into DateTime and add CalendarDate (for date).

Same for DateRange.

For compatiblity Date is an alias for DateTime. The idea is to rename CalendarDate into Date later on.

see #346 and #168

@philippjfr
Copy link
Member

This looks great to me. @jbednar @jlstevens Any objections?

@randomstuff
Copy link
Contributor Author

(Merged the first two commits)

param/__init__.py Outdated Show resolved Hide resolved
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

This all looks good to me. I can't see any problems with backwards compatibility, and this is a much better approach going forwards. I'm happy for it to be merged once the above minor docs issues are either addressed or resolved as not relevant. Thanks so much for submitting this, and I'm sorry for having taken so long to review it (blame SciPy2019 and all of its amazingness!).

@randomstuff
Copy link
Contributor Author

randomstuff commented Jul 25, 2019

If this change is merged I'm not sure it makes sense to allow Datetime and DateTimeRange to hold date values instead of datetime ones. I find it's pretty strange:

import datetime

import param

class Foo(param.Parameterized):
    p = param.DateTime()

foo = Foo()
foo.p = datetime.date.today()
print(repr(foo.p)) # => datetime.date(2019, 7, 25)

Changing this would be a breaking change however.

@randomstuff
Copy link
Contributor Author

Fixed the documentation of the changes which has some confusing regarding datetime vs date, etc.

@randomstuff
Copy link
Contributor Author

Another solution would be to keep Date dand DateRange for now (with the idea or replacing them later on with CalendarDate and CalendarDateTime) which would support both for now and add new DateTime and DateTimeRange which would only accept datetime

@randomstuff
Copy link
Contributor Author

Updated to this alternative proposition:

  • keep Date unmodified;
  • CalendarDate for date-only.

I'd be tempted to add (in other PRs):

  • NaiveDateTime for naive date times (no timezone information);
  • DateTime (?) for non-naive datetime;
  • Time for (naive) time.

Date and DateRange as kepts for hybrid date/datetime for compatibility.
@philippjfr
Copy link
Member

Thanks @randomstuff! Sorry for the long review cycle. This is hugely appreciated.

@philippjfr philippjfr merged commit cad5608 into holoviz:master Jul 31, 2019
@randomstuff randomstuff deleted the date_and_datetime branch July 31, 2019 18:38
@randomstuff
Copy link
Contributor Author

randomstuff commented Jul 31, 2019

I was looking of the different panel widgets and how they would behave if using CalendarDate instead of Date this change and I discovered that datetime is actually a subclass of date. 😲

As a consequence, CalendarDate can currenly hold datetime values which is not what I intended. 😄 The only difference between Date and CalendarDate is that the latter cannot hold a np.datetime64 value (because np.datetime64 is not a subclass of date). This is somewhat nice because np.datetime64 does not behave as a date at all (no .month, etc.).

Conclusion:

  • As datetime is a subclass of date, it is kind-of expected that the Date/CalendarDate parameters would accept datetime values so the Date parameter type is actually OK in this regard.

  • I wonder if it would not be useful to convert datetime to date on assignment (either for Date/CalendarDate).

  • I'm not sure CalendarDate is actually any useful in its current state and I'd vote for reverting it for now (at least before release if no clear plan as been found).

  • Allowing Date parameters to hold np.datetime64 could be problematic because np.datetime64 does not behave as date at all. It might be more useful to convert np.datetime64 to datetime/date automatically when assigning (?).

philippjfr pushed a commit that referenced this pull request Aug 2, 2019
Date and DateRange as kepts for hybrid date/datetime for compatibility.
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.

3 participants