search

korma / Korma

Tasty SQL for Clojure.
http://sqlkorma.com
1.48k stars 222 forks source link

Field of record support extensible data type #333

Closed gfZeng closed 8 years ago

gfZeng commented 8 years ago

Please see this PR

gfZeng commented 8 years ago

Hi @immoh

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

immoh commented 8 years ago

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 https://github.com/korma/Korma/pull/333#issuecomment-161234434.

immoh commented 8 years ago

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.