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 parse(::AbstractString, ::DateFormat) #20880

Closed
wants to merge 1 commit into from
Closed

Conversation

omus
Copy link
Member

@omus omus commented Mar 3, 2017

Properly deprecates the Dates.parse(::AbstractString, ::DateFormat) -> Array{Period} function and replaces it with parse(::Type{Array{Period}}, ::AbstractString, ::DateFormat) -> Array{Period}. Note that the replacement slightly differs from the original function by returning the periods in the order of the DateFormat tokens and in period sorted order.

Fixes #20876

Properly deprecates the `Dates.parse(::AbstractString, ::DateFormat)`
function which returned an `Array{Period}`. Replaces this function with
`parse(::Type{Array{Period}}, ::AbstractString, ::DateFormat)`. Slightly
differs from the original function by returning the periods in the
order of the `DateFormat` and not in reverse sorted order.
@omus omus added backport pending 0.6 dates Dates, times, and the Dates stdlib module labels Mar 3, 2017
@omus
Copy link
Member Author

omus commented Mar 3, 2017

I possibly could have reworked the original tryparse_internal instead of making a new method but it seemed safest to avoid refactoring code this close to release.

@tkelman
Copy link
Contributor

tkelman commented Mar 3, 2017

there's no separate branch yet

@@ -200,3 +200,58 @@ function Base.parse(::Type{DateTime}, s::AbstractString, df::typeof(ISODateTimeF
@label error
throw(ArgumentError("Invalid DateTime string"))
end

@generated function tryparse_internal{S, F}(::Type{Array{Period}}, str::AbstractString, df::DateFormat{S, F}, raise::Bool=false)
Copy link
Contributor

Choose a reason for hiding this comment

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

wrap long lines

@tkelman tkelman added this to the 0.6.0 milestone Mar 4, 2017
@shashi
Copy link
Contributor

shashi commented Mar 4, 2017

It seems this is pretty much doing the same thing as

@generated function tryparse_internal{T<:TimeType, S, F}(::Type{T}, str::AbstractString, df::DateFormat{S, F}, raise::Bool=false)
? except is 1) not reordering the output 2) not wrapping the numbers in Period types 3) returning an Array instead of a tuple. Is being able to return values unsorted really useful? The deprecation suggests doing the sort anyway.

This new method could just be

function Base.parse{T<:Array{Period}}(::Type{T}, str::AbstractString, df::DateFormat)
     Period[get(tryparse_internal(DateTime, str, df, true))...]
end

if sorting the periods automatically is not a problem.

returning an array in a function like this seems like a performance trap, we should probably consider just returning that tuple.

@omus
Copy link
Member Author

omus commented Mar 6, 2017

@shashi additionally the revised function is only returning the periods specified in the format:

julia> Base.Dates.tryparse_internal(DateTime, "Apr 1, 24:00", dateformat"uuu dd, HH:MM", true)
Nullable{NTuple{7,Int64}}((1, 4, 1, 24, 0, 0, 0))

julia> Base.Dates.parse(Array{Dates.Period}, "Apr 1, 24:00", dateformat"uuu dd, HH:MM")
4-element Array{Base.Dates.Period,1}:
 4 months 
 1 day    
 24 hours 
 0 minutes

This is the reason I ended up returning an Array{Period} instead of a tuple.

Is being able to return values unsorted really useful? The deprecation suggests doing the sort anyway.

I consider that the old behaviour of always returning the periods in reverse sorted order was unnecessary and made it impossible to get the values in the order of the format. The deprecation suggests using a sort only so that deprecated code is 100% backwards compatible.

@StefanKarpinski
Copy link
Member

Is this superseded by your newer PR or should we go ahead with this?

@omus
Copy link
Member Author

omus commented Mar 16, 2017

Is this superseded by your newer PR or should we go ahead with this?

This PR is superseded by #20952

@omus omus closed this Mar 16, 2017
@omus omus deleted the cv/parse-periods branch March 16, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dates Dates, times, and the Dates stdlib module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants