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

Update to crystal 0.25.0 #228

Merged
merged 10 commits into from
Jun 15, 2018
Merged

Update to crystal 0.25.0 #228

merged 10 commits into from
Jun 15, 2018

Conversation

faustinoaq
Copy link
Contributor

@faustinoaq faustinoaq commented Jun 15, 2018

Minimally addresses issues required to update Granite to Crystal 0.25.0

Read the changelog for anything I missed. https://github.com/bcardiff/crystal/blob/changelog/0.25.0/CHANGELOG.md

Fixes #226

@faustinoaq faustinoaq changed the title Update pg shard dependency Update to crystal 0.25.0 Jun 15, 2018
@Blacksmoke16
Copy link
Contributor

@faustinoaq We probably should also address other changes specified in #226.

@robacarp
Copy link
Member

I believe this addresses all breaking changes for Granite to update to 0.25.

This represents a catastrophic breaking change for anyone using sqlite.

Sqlite, Mysql, Postgres all handle storing datetimes differently, and Crystal has changed the way it handles this stuff. Fun times.

The updates to the time code are basically:

  • Standardize Granite on Granite::DATETIME_FORMAT where previously %F %X was scattered everywhere.
  • Granite::DATETIME_FORMAT now includes a timezone offset: %F %X%z. Which means that times will now be rendered as '2018-06-15 12:19:58+00:00' instead of without the offset part.
  • Most notably, timestamps stored in a sqlite database will not have the timezone and will throw an exception on read.
  • Mysql returns a Time with a location of "local" instead of "unspecified". The mysql shard seems to have support for converting to UTC, but granite does not appear take advantage of this.
  • Clean up a few tests.

cc @drujensen @damianham (I believe you've dealt with sqlite timestamps in the past?)

Copy link
Member

@drujensen drujensen left a comment

Choose a reason for hiding this comment

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

@robacarp yes, I just disabled the timestamps to handle sqlite. It is a pita :-/

@@ -23,7 +23,7 @@ module Granite::Querying
if @@adapter.class.name == "Granite::Adapter::Sqlite"
# sqlite3 does not have timestamp type - timestamps are stored as str
# will break for null timestamps
self.\{{name.id}} = Time.parse(result.read(String), "%F %X" )
self.\{{name.id}} = Time.parse(result.read(String), Granite::DATETIME_FORMAT)
Copy link
Member

Choose a reason for hiding this comment

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

nice change to Granite::DATETIME_FORMAT 💯

@drujensen drujensen merged commit 64b3e31 into master Jun 15, 2018
@drujensen drujensen deleted the fa/update-pg branch June 15, 2018 20:16
@damianham
Copy link
Contributor

why not rescue the exception for existing time strings and try to parse with "%F %X" ?

@faustinoaq
Copy link
Contributor Author

why not rescue the exception for existing time strings and try to parse with "%F %X" ?

Interesting idea, Would this solve the SQLite issue?

^^ @robacarp @drujensen

@drujensen
Copy link
Member

drujensen commented Jun 17, 2018

I think this might work with one caveat:

We either need to store the time as UTC as a convention or we need to include the timezone in the stored string. You should always store the timezone implicitly or explicitly to get the right results.

I prefer we store the timezone in the string explicitly. The advantage is you can determine the timezone that was originally used by the client/server when it was originally stored. If you convert it to UTC before storing, you lose that information.

@robacarp
Copy link
Member

I agree, it makes sense for Granite to store the timezone as provided in the Time object given. In the event that it makes more sense for an application timezone to always be UTC, the appdev can make that happen easy enough.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants