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

fix: datetime tryparse (#265) #267

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

Arkoniak
Copy link

@Arkoniak Arkoniak commented Mar 8, 2023

I've tested it locally with Julia versions 1.7.2 and 1.8.1 and it works correctly. Not sure, whether it make sense to add any specific tests, since runtests works with postgres database as a whole and should cover this function (for latest versions of Julia at least).

src/parsing.jl Outdated Show resolved Hide resolved
@iamed2
Copy link
Collaborator

iamed2 commented Mar 9, 2023

Yup no need to add specific tests.

This should be added to the ZonedDateTime and UTCDateTime parsing methods as well though, to replicate the same behaviour (I assume it also happens for those types?). For ZonedDateTime you can put the whole for-loop in the try-catch.

@Arkoniak
Copy link
Author

Well, in the end, I moved all @static to utility function, this allows for

  1. having clear code in main function
  2. it'll be easy to remove this fix in the future (just delete _tryparse and change all _tryparse calls to tryparse

Also, I've updated pqparse(::Type{Time}, str::AbstractString) function to use the same approach.

I am using tryparse in catch block, because for some reason for ZonedDateTime tryparse is working properly in 1.7

@Arkoniak
Copy link
Author

Well, in the end, I moved all @static to utility function, this allows for

  1. having clear code in main function
  2. it'll be easy to remove this fix in the future (just delete _tryparse and change all _tryparse calls to tryparse

Also, I've updated pqparse(::Type{Time}, str::AbstractString) function to use the same approach.

I am using tryparse in catch block, because for some reason for ZonedDateTime tryparse is working properly in 1.7

@Arkoniak
Copy link
Author

Sorry for bothering @iamed2 , but is there anything else that should be done in this PR?

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