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

Add timezone to DateTime column type #109

Closed
wants to merge 1 commit into from

Conversation

Arcnor
Copy link

@Arcnor Arcnor commented May 24, 2017

This PR basically adds timezone for the DATETIME type on all supported DBs. It also fixes at least a bug when retrieving a DateTime type from the DB (it tries to convert a DateTime to a DateTime by parsing it through the formatter, but it fails when doing so. Also, this is obviously not necessary)

else
"'${DEFAULT_DATE_STRING_FORMATTER.print(dateTime)}'"
}

override fun valueFromDB(value: Any): Any = when(value) {
is DateTime -> value
Copy link
Author

Choose a reason for hiding this comment

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

The only "non-obvious" change this makes is that the returned instance is the same one, but in all other branches we create a new DateTime object. Not sure if that behavior needs to be preserved or not.

Copy link
Contributor

@msrd0 msrd0 Jul 19, 2017

Choose a reason for hiding this comment

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

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the tip!

@Arcnor
Copy link
Author

Arcnor commented May 24, 2017

I was also tempted to add a TIME type as part of this, but I'm not sure what the preferred way of doing that will be. I think there should be a DateColumnType that only deals with dates, then a TimeColumnType, and then DateTimeColumnType, because the timezone concept is only applicable in some cases. It might be a bit more verbose, but might also be easier to understand. Anyway, not part of this PR.

@Arcnor
Copy link
Author

Arcnor commented Jul 19, 2017

Is this project accepting contributions? This PR has been opened for a month, but there is not a single comment on it (besides @msrd0, thanks again).

@msrd0
Copy link
Contributor

msrd0 commented Jul 20, 2017

I once had a PR merged, but I haven't read a single comment from the developers, neither on PRs nor on issues

@Tapac
Copy link
Contributor

Tapac commented Jul 20, 2017

@Arcnor , @msrd0 , sorry for long reply.
I have some question about how it works? Does it use DBMS timezone when store datetimes?
What timezone will be used when read from database: application one or database one if they differ?

@YakovSirotkin
Copy link
Contributor

@Arcnor Could you please provide an example where withTimezone parameter is true? And for which databases in makes sense?

@alsofr
Copy link

alsofr commented Sep 25, 2020

Would it make sense to fix the conflicts and merge this pull request? There still isn't any support for the date time with timezone Postgres type.
Are we supposed not to use datetimes and use timestamps instead?

I don't really understand the concept of datetime without timezone, what is the use case for this?

@joc-a
Copy link
Collaborator

joc-a commented Jul 27, 2023

Hey @Arcnor. Thank you for your contribution. This feature has now been implemented here #1787.

@joc-a joc-a closed this Jul 27, 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.

6 participants