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

Add correct String/ChannelBuffer injections #257

Merged
merged 2 commits into from
Mar 2, 2015

Conversation

franklinhu
Copy link
Contributor

The String and ChannelBuffer injections provided in storehaus-mysql are flipped (there's always a mapping to MySqlValue but not always back). Added injections in the right direction and deprecated the existing ones.

  • Add String2MySqlValueInjection and ChannelBuffer2MySqlValueInjection
  • Deprecate MySqlStringInjection and MySqlCbInjection

@johnynek
Copy link
Collaborator

Yes, it does look like the original ones are a bit ill-formed. Can you add some tests here? The IsInjection property is pretty easy to use as long as you have the Gen/Arbitrary for the types in question.

@franklinhu
Copy link
Contributor Author

It looks like IsInjection is only accessible in bijection-core?https://github.com/twitter/bijection/blob/23b7978fd8a4948ad294aa44fda35db5747950bd/bijection-core/src/test/scala/com/twitter/bijection/BaseProperties.scala#L66-L68

It also seems like we could remove the storehaus MySqlValue wrapper over the Finagle types since bijection-finagle-mysql exists now?

@johnynek
Copy link
Collaborator

sadly those BaseProperties are not exported. You'll have to do something like:

forAll { (i: Int) =>
  injection.invert(injection(i)) == Success(i)
}

is good enough.

About bijection-finagle-mysql: yes we should remove the repeated code here (once the tests are passing on that and we have a new published version). For now let's make an issue with the correct links.

@franklinhu
Copy link
Contributor Author

Added the tests, and ended up discovering a bug in how ChannelBuffers are converted to MySQL types.

johnynek added a commit that referenced this pull request Mar 2, 2015
Add correct String/ChannelBuffer injections
@johnynek johnynek merged commit 8b6fd5c into twitter:develop Mar 2, 2015
@johnynek
Copy link
Collaborator

johnynek commented Mar 2, 2015

Thanks for this! Looks good.

@franklinhu franklinhu deleted the franklin-mysql-injections branch March 2, 2015 22:45
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.

3 participants