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

Switch time string parsing to tryparse to handle Julia 1.6.6 change #248

Merged
merged 1 commit into from
May 9, 2022

Conversation

Wynand
Copy link
Contributor

@Wynand Wynand commented May 5, 2022

When parse(Time, str) fails for str="13:13:13142" on Ubuntu Julia 1.6.6, an ArgumentError is thrown instead of an InexactError, but when using tryparse nothing is returned

This switches updates the logic to handle the new return type


Reference links:

@Wynand Wynand marked this pull request as draft May 5, 2022 19:16
src/parsing.jl Outdated Show resolved Hide resolved
@Wynand
Copy link
Contributor Author

Wynand commented May 5, 2022

Turns out I was testing locally with Julia 1.6.5, and this fails locally with 1.6.6

src/parsing.jl Outdated
if !(err isa InexactError)
rethrow(err)
try
return parse(Time, _trunc_seconds(str))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a fan of how errors from _trunc_seconds are effectively ignored by this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would it be worth while to just always call _trunc_seconds on str?

IE, to change this function to

pqparse(::Type{Time}, str::AbstractString) = parse(Time, _trunc_seconds(str))

@iamed2
Copy link
Collaborator

iamed2 commented May 5, 2022

If it generates an InexactError on 1.6.5 AND current stable I think we have a case for this being a Julia bug in 1.6.6

@Wynand
Copy link
Contributor Author

Wynand commented May 5, 2022

If it generates an InexactError on 1.6.5 AND current stable I think we have a case for this being a Julia bug in 1.6.6

My best guess is that this was introduced by this change: JuliaLang/julia#44004

When I test this on Julia 1.7 I get and InexactError, and the stacktrace uses tryparsenext:

julia> parse(Time, "13:13:13.13124")
ERROR: InexactError: convert(Dates.Decimal3, 13124)
Stacktrace:
 [1] tryparsenext
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/io.jl:153 [inlined]
 [2] tryparsenext
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/io.jl:41 [inlined]
 [3] macro expansion
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:64 [inlined]
 [4] tryparsenext_core(str::String, pos::Int64, len::Int64, df::DateFormat{Symbol("HH:MM:SS.s"), Tuple{Dates.DatePart{'H'}, Dates.Delim{Char, 1}, Dates.DatePart{'M'}, Dates.Delim{Char, 1}, Dates.DatePart{'S'}, Dates.Delim{Char, 1}, Dates.DatePart{'s'}}}, raise::Bool)
   @ Dates /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:38
 [5] macro expansion
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:150 [inlined]
 [6] tryparsenext_internal
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:125 [inlined]
 [7] parse
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:282 [inlined]
 [8] parse(::Type{Time}, str::String)
   @ Dates /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:281
 [9] top-level scope

This change isn't part of 1.7: https://github.com/JuliaLang/julia/blob/v1.7.2/stdlib/Dates/src/io.jl#L153
but it looks like it will be: https://github.com/JuliaLang/julia/commits/kc/1.7.3/stdlib/Dates/src/io.jl

@Wynand Wynand changed the title naively try truncating time string when parsing fails Switch time string parsing to tryparse to handle Julia 1.6.6 change May 5, 2022
@Wynand Wynand marked this pull request as ready for review May 5, 2022 20:32
@Wynand Wynand requested a review from iamed2 May 5, 2022 20:32
@iamed2
Copy link
Collaborator

iamed2 commented May 5, 2022

I'm confused; does tryparse throw an error sometimes? I would have thought we could get rid of the try-catch block if we used tryparse.

@Wynand
Copy link
Contributor Author

Wynand commented May 5, 2022

I'm confused; does tryparse throw an error sometimes? I would have thought we could get rid of the try-catch block if we used tryparse.

Yes, in Julia 1.6.5 and 1.7.2 it throws InexactError, but in 1.6.6 (and probably 1.7.3) is just returns nothing.
This is because the MR linked in the description has been backported to 1.6 but hasn't been backported to 1.7

1.6.6:

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.6.6 (2022-03-28)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using Dates

julia> tryparse(Time, "13:13:13.13142")

julia>

1.7.2:

               _
   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.7.2 (2022-02-06)
 _/ |\__'_|_|_|\__'_|  |  Official https://julialang.org/ release
|__/                   |

julia> using Dates

julia> tryparse(Time, "13:13:13.13142")
ERROR: InexactError: convert(Dates.Decimal3, 13142)
Stacktrace:
 [1] tryparsenext
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/io.jl:153 [inlined]
 [2] tryparsenext
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/io.jl:41 [inlined]
 [3] macro expansion
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:64 [inlined]
 [4] tryparsenext_core(str::String, pos::Int64, len::Int64, df::DateFormat{Symbol("HH:MM:SS.s"), Tuple{Dates.DatePart{'H'}, Dates.Delim{Char, 1}, Dates.DatePart{'M'}, Dates.Delim{Char, 1}, Dates.DatePart{'S'}, Dates.Delim{Char, 1}, Dates.DatePart{'s'}}}, raise::Bool)
   @ Dates /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:38
 [5] macro expansion
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:150 [inlined]
 [6] tryparsenext_internal
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:125 [inlined]
 [7] tryparse
   @ /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:290 [inlined]
 [8] tryparse(::Type{Time}, str::String)
   @ Dates /usr/local/julia/share/julia/stdlib/v1.7/Dates/src/parse.jl:289
 [9] top-level scope
   @ REPL[2]:1

julia> 

@iamed2
Copy link
Collaborator

iamed2 commented May 6, 2022

Let's put the new behaviour and old behaviour in there and guard with a @static if VERSION ... block, so that we can remove the error-handling when it's no longer necessary.

@Wynand
Copy link
Contributor Author

Wynand commented May 9, 2022

Let's put the new behaviour and old behaviour in there and guard with a @static if VERSION ... block, so that we can remove the error-handling when it's no longer necessary.

I've added some version based logic

@Wynand Wynand force-pushed the wy/fix-ubuntu-truncating branch 2 times, most recently from 3a1992c to 565265d Compare May 9, 2022 16:27
@iamed2
Copy link
Collaborator

iamed2 commented May 9, 2022

Bump the version to 1.13 and we can merge and tag

@Wynand Wynand force-pushed the wy/fix-ubuntu-truncating branch from dd7e1aa to e94033c Compare May 9, 2022 17:58
@Wynand
Copy link
Contributor Author

Wynand commented May 9, 2022

Bump the version to 1.13 and we can merge and tag

updated

@iamed2 iamed2 merged commit 5ba8155 into master May 9, 2022
@iamed2 iamed2 deleted the wy/fix-ubuntu-truncating branch May 9, 2022 19:17
@Arkoniak Arkoniak mentioned this pull request Mar 2, 2023
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.

2 participants