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

Read Slice(UInt8) as String if given as type #221

Merged
merged 2 commits into from
Jan 12, 2021

Conversation

matthewmcgarvey
Copy link
Contributor

Fixes #43

In order to support the citext extension in Avram, we need to be able to treat the column as a String. Two solutions were laid out in the connected issue:

  • A: Calling rs.read(String) on a string-like column (citext, enum or similar), it should return a String.
  • B: Calling rs.read on a string-like column, it should probably also return a String.

B was considered the more "complete solution" but also the more complicated. Since it's been 4 years since the issue was opened and over a year since the potential solutions were laid out, I went with the more immediate option A. If the read type is Slice(UInt8), which is the type of a citext column, and #read was called with a type of String, then convert it to String. This helps with citext interop and I'm sure it helps with other column types as well.

@matthewmcgarvey
Copy link
Contributor Author

This is related to #89 which is attempting a more complete implementation laid out here #43 (comment) but stalled out.

@will
Copy link
Owner

will commented Dec 31, 2020

Thanks! This looks good to me. I don’t myself use citext though, so I'm going to leave this open for a couple of days in case anyone has comments. But I expect to merge this, then do a release sometime early next week.

Copy link
Collaborator

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

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

At some point conversions like this should be handled in crystal-db directly as long as they are type-safe and without configuration. But to this some changes in the design need to be made. So, this is they for now.

@bcardiff
Copy link
Collaborator

bcardiff commented Jan 9, 2021

Oh! I remember. Double check that when using rs.read(String?) the conversion will work. It will probably wont. In crystal-sqlite3 there are a couple of these IIRC.

@will will merged commit d4abb0c into will:master Jan 12, 2021
@matthewmcgarvey matthewmcgarvey deleted the matthewmcgarvey/issue43 branch January 12, 2021 19:37
@will
Copy link
Owner

will commented Jan 12, 2021

Thanks!

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.

CITEXT type is understood as Slice instead of String in 0.7.1
5 participants