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

Field of record support extensible data type #333

Closed
wants to merge 6 commits into from

Conversation

gfZeng
Copy link
Contributor

@gfZeng gfZeng commented Nov 25, 2015

Please see this PR

@gfZeng
Copy link
Contributor Author

gfZeng commented Dec 2, 2015

Hi @immoh

I submitted this PR at 7 days ago.
there has any problem?

@immoh
Copy link
Member

immoh commented Dec 2, 2015

I haven't taken a look at it yet, will do so in the next few days.

ke 2. joulukuuta 2015 klo 11.29 Isaac Zeng notifications@github.com
kirjoitti:

Hi @immoh https://github.com/immoh

I submitted this PR at 7 days ago.
there has any problem?


Reply to this email directly or view it on GitHub
#333 (comment).

@gfZeng gfZeng force-pushed the master branch 2 times, most recently from d610b98 to 4697f47 Compare January 5, 2016 15:29
@immoh
Copy link
Member

immoh commented Jan 14, 2016

My sincerest apologies for not finding the time to look at this earlier.

I'm not quite sure why you chose to close this PR. Perhaps because of extra commits? You can always just force push to your branch and the PR should be updated accordingly.

Commenting on the first commit: I don't like the approach of picking two specific places and manually checking if the value satisfies ISQLValue. I think all values form Korma should be passed to java.jdbc as-is, in all statement types not just insert. This conflicts with the fact that Korma happens to use maps to handle special values and needs to be solved somehow. This approach requires larger changes but I don't think PR offers enough big win to be accepted as it is. 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.

2 participants