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 json column support mysql(5.7~) #201

Merged
merged 11 commits into from Nov 8, 2017

Conversation

Projects
None yet
6 participants
@hashijun
Contributor

hashijun commented Oct 17, 2017

fixes #119 . (This pull request is re-creation for #176 .)

Encode and decode library for json can choose by opts[:json_library] (Default set to Poison).

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 17, 2017

Coverage Status

Coverage decreased (-0.2%) to 65.854% when pulling decc63a on hashijun:work/add_json_support_on_v0.8.3 into 59d7ae0 on xerions:master.

coveralls commented Oct 17, 2017

Coverage Status

Coverage decreased (-0.2%) to 65.854% when pulling decc63a on hashijun:work/add_json_support_on_v0.8.3 into 59d7ae0 on xerions:master.

@fishcakez

This looks great. I added a few comments to tidy up some minor details.

It's unfortunate that can't encode a parameter as JSON. I imagine this is the same scenario as #198 (comment), where we would need to use types sent by the database on prepare. I have not got to looking at that yet but perhaps someone else will before I get chance.

Show outdated Hide outdated mix.exs Outdated
Show outdated Hide outdated test/text_query_test.exs Outdated
Show outdated Hide outdated lib/mariaex/protocol.ex Outdated
Show outdated Hide outdated test/query_test.exs Outdated
@hashijun

This comment has been minimized.

Show comment
Hide comment
@hashijun

hashijun Oct 19, 2017

Contributor

@fishcakez

Thanks for your comment! I'll fix my code based on review.

Contributor

hashijun commented Oct 19, 2017

@fishcakez

Thanks for your comment! I'll fix my code based on review.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 65.854% when pulling f5703b2 on hashijun:work/add_json_support_on_v0.8.3 into 59d7ae0 on xerions:master.

coveralls commented Oct 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 65.854% when pulling f5703b2 on hashijun:work/add_json_support_on_v0.8.3 into 59d7ae0 on xerions:master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Oct 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 65.854% when pulling f5703b2 on hashijun:work/add_json_support_on_v0.8.3 into 59d7ae0 on xerions:master.

coveralls commented Oct 22, 2017

Coverage Status

Coverage decreased (-0.2%) to 65.854% when pulling f5703b2 on hashijun:work/add_json_support_on_v0.8.3 into 59d7ae0 on xerions:master.

@hashijun

This comment has been minimized.

Show comment
Hide comment
@hashijun

hashijun Nov 3, 2017

Contributor

@fishcakez

I fixed my changeset based on your review. Is there anything to improve?

Contributor

hashijun commented Nov 3, 2017

@fishcakez

I fixed my changeset based on your review. Is there anything to improve?

@mgwidmann

This comment has been minimized.

Show comment
Hide comment
@mgwidmann

mgwidmann Nov 5, 2017

waiting on this to release 👍

mgwidmann commented Nov 5, 2017

waiting on this to release 👍

@fishcakez

This comment has been minimized.

Show comment
Hide comment
@fishcakez

fishcakez Nov 5, 2017

Contributor

@hashijun I think you addressed my points nicely but I am not a maintainer of this repo, and am unsure how I feel about supporting JSON in rows but not in parameters.

Contributor

fishcakez commented Nov 5, 2017

@hashijun I think you addressed my points nicely but I am not a maintainer of this repo, and am unsure how I feel about supporting JSON in rows but not in parameters.

@liveforeverx liveforeverx merged commit 9b99297 into xerions:master Nov 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@liveforeverx

This comment has been minimized.

Show comment
Hide comment
@liveforeverx
Member

liveforeverx commented Nov 8, 2017

Thank you! @hashijun and @fishcakez

@hashijun hashijun deleted the hashijun:work/add_json_support_on_v0.8.3 branch Nov 9, 2017

@m-koepke

This comment has been minimized.

Show comment
Hide comment
@m-koepke

m-koepke Nov 9, 2017

@hashijun How would writing the JSON work? If I understand correctly I need to pass an already JSON encoded String? The original PR also supported encoding JSON: https://github.com/xerions/mariaex/pull/197/files#diff-cd3b88763eeb706bf700cab0689dc94cR103

m-koepke commented Nov 9, 2017

@hashijun How would writing the JSON work? If I understand correctly I need to pass an already JSON encoded String? The original PR also supported encoding JSON: https://github.com/xerions/mariaex/pull/197/files#diff-cd3b88763eeb706bf700cab0689dc94cR103

@hashijun

This comment has been minimized.

Show comment
Hide comment
@hashijun

hashijun Nov 11, 2017

Contributor

@m-koepke That would be further problem (also noted in #204). I'll try in the near future, but it's advanced task so it may take long time.

Contributor

hashijun commented Nov 11, 2017

@m-koepke That would be further problem (also noted in #204). I'll try in the near future, but it's advanced task so it may take long time.

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