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 DateTime() Date() Time() #23724

Merged
merged 2 commits into from Jan 23, 2018
Merged

Conversation

a-hofer
Copy link
Contributor

@a-hofer a-hofer commented Sep 15, 2017

Will be now after julia 0.7.

@omus omus added the domain:dates Dates, times, and the Dates stdlib module label Sep 15, 2017
@omus
Copy link
Member

omus commented Sep 15, 2017

cc: @quinnj @StefanKarpinski

@omus omus added the kind:deprecation This change introduces or involves a deprecation label Sep 15, 2017
@omus
Copy link
Member

omus commented Sep 15, 2017

Commits should be squashed upon merging.

DateTime() = now()
Date() = today()
Time() = Time(now())
end
Copy link
Member

Choose a reason for hiding this comment

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

We'll have to update the documentation after the release of 0.7.

@fredrikekre
Copy link
Member

This will cause

WARNING: Method definition (::Type{Base.Dates.Date})() in module Dates at dates/types.jl:318 overwritten at deprecated.jl:55.
WARNING: Method definition (::Type{Base.Dates.Time})() in module Dates at dates/types.jl:319 overwritten at deprecated.jl:55.

during sysimg build and

julia> DateTime()
WARNING: DateTime() is deprecated, use DateTime(1) instead.
Stacktrace:
 [1] depwarn(::String, ::Symbol) at ./deprecated.jl:68
 [2] DateTime() at ./deprecated.jl:56
 [3] eval(::Module, ::Expr) at ./repl/REPL.jl:3
 [4] eval_user_input(::Any, ::Base.REPL.REPLBackend) at ./repl/REPL.jl:69
 [5] macro expansion at ./repl/REPL.jl:100 [inlined]
 [6] (::getfield(Base.REPL, Symbol("##1#2")){Base.REPL.REPLBackend})() at ./event.jl:96
while loading no file, in expression starting on line 0
0001-01-01T00:00:00

on master as soon as the version is bumped. Would it be better to add the new methods in a separate PR after the deprecations are removed? Then the docs could be updated in said PR also, so we don't forget about it?

@StefanKarpinski
Copy link
Sponsor Member

@frederikekre's plan sounds like a good one to me. @a-hofer, if you're willing to do that, please go ahead, otherwise, @fredrikekre, could you push commits to this branch to "make it so"?

@fredrikekre
Copy link
Member

fredrikekre commented Sep 18, 2017

Okay, I have fixed up the PR. But as I did so I thought about two things:

  1. I don't think we should define the new methods DateTime() = now(), Date() = today() and Time() = Time(now()). It is much less confusing to be explicit and use now(), today() and Time(now()) directly. Then you don't have to look up in the documentation what the zero arg methods do.

  2. With #1 in mind; what should the zero arg constructors do? Looking at the documentation for all of them it states:

"""
    DateTime(periods::Period...) -> DateTime

Construct a `DateTime` type by `Period` type parts. Arguments may be in any order. DateTime
parts not provided will default to the value of `Dates.default(period)`.
"""
"""
    Date(period::Period...) -> Date

Construct a `Date` type by `Period` type parts. Arguments may be in any order. `Date` parts
not provided will default to the value of `Dates.default(period)`.
"""
"""
    Time(period::TimePeriod...) -> Time

Construct a `Time` type by `Period` type parts. Arguments may be in any order. `Time` parts
not provided will default to the value of `Dates.default(period)`.
"""

It is clearly documented what the periods default to, Dates.default(period). It would be kinda weird if you could supply an number of periods, and get default values for the ones you are missing, but you must supply at least one. This does not make sense to me.

With this in mind I don't think this PR is accomplishing anything really, and I argue that status quo is the best.

@omus
Copy link
Member

omus commented Sep 18, 2017

One of the issues here is that the now and today function are not mentioned in the Date and DateTime section of the manual (it is of course mentioned in the stdlib Dates section). Additionally, adding a cross reference in the DateTime docstring to the now function could also help newcomers out.

@omus
Copy link
Member

omus commented Sep 18, 2017

If we decide not to go with DateTime() = now() I could still see us deprecating DateTime() just to avoid confusion. I believe the DateTime(::Period...) constructor was originally made to support the pre-Julia 0.6 date time string parsing which would always at least supply a single period.

@iamed2
Copy link
Contributor

iamed2 commented Sep 18, 2017

For reference:

MATLAB's datenum and datevec do not have 0-arg forms. MATLAB's new datetime does, and it means the current date and time.
Python's datetime and date do not have 0-arg forms.

@a-hofer
Copy link
Contributor Author

a-hofer commented Sep 18, 2017

For context I am just learning Julia, and I wanted to create a 'now' DateTime, and as @omus mentioned it was not obvious how to do so from the documentation. Personally I think it would be much more helpful if the empty constructor had this functionality, because this seems like a very common use case to me, at least much more common than needing DateTime(1). Whatever you guys think is best is fine with me, just figured you might want a newbie perspective. Mentioning the now() function in the documentation might have a similar effect.

@StefanKarpinski
Copy link
Sponsor Member

I don't think we should define the new methods DateTime() = now(), Date() = today() and Time() = Time(now()). It is much less confusing to be explicit and use now(), today() and Time(now()) directly. Then you don't have to look up in the documentation what the zero arg methods do.

I was actually thinking that it might make sense and go completely the other direction: deprecate now() and today() in favor of DateTime() and Date() and of course Time() then becomes a completely natural third constructor for the current time.

@fredrikekre
Copy link
Member

I think I will leave this PR in the hands of Date's people, I only thought it would be a good idea to rethink this :)

@ExpandingMan
Copy link
Contributor

By the way, currently Date("") returns the date 0001-01-01 instead of throwing an error, this PR changes this, correct? DateTime("") throws an error as it should.

@JeffBezanson
Copy link
Sponsor Member

Bump --- do we want this in 1.0? Needs to be rebased after Dates was moved to stdlib.

@omus omus added the status:triage This should be discussed on a triage call label Jan 23, 2018
@omus
Copy link
Member

omus commented Jan 23, 2018

Changes have been rebased to use the stdlib Dates. I'm in favor of adding this deprecation but I thought it may be best for triage to discuss.

@StefanKarpinski
Copy link
Sponsor Member

No triage required, we should do this.

@omus omus removed the status:triage This should be discussed on a triage call label Jan 23, 2018
@omus omus merged commit 1888eb6 into JuliaLang:master Jan 23, 2018
@omus omus deleted the ah/EmptyDatesNow branch January 23, 2018 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:dates Dates, times, and the Dates stdlib module kind:deprecation This change introduces or involves a deprecation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants