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

Fix for Ad-Hoc Case Class producing Dynamic Queries #1000

Merged
merged 1 commit into from
Dec 21, 2017

Conversation

deusaquilus
Copy link
Collaborator

Fixes #992

Problem

Whenever Ad-Hoc case classes are used, AST produces a Dynamic Query which should not be the case.

Solution

Add case for Ad-Hoc Case Class in Unliftables.

Notes

Please note that I also added a test to IsDynamicSpec which is supposed to test this functionality but the IsDynamic method will produce a 'false' (i.e. it thinks it's not dynamic) even when line 154 in Unliftables in commented out. I think IsDynamic doesn't work properly but not sure what to do about it.

Checklist

  • Unit test all changes
  • Update README.md if applicable
  • Add [WIP] to the pull request title if it's work in progress
  • Squash commits that aren't meaningful changes
  • Run sbt scalariformFormat test:scalariformFormat to make sure that the source files are formatted

@getquill/maintainers

@deusaquilus deusaquilus changed the title Fix for #992 Ad-Hoc Case Class producing Dynamic Queries Fix for Ad-Hoc Case Class producing Dynamic Queries Dec 21, 2017
@mosyp
Copy link
Collaborator

mosyp commented Dec 21, 2017

@deusaquilus Thanks for contribution and sorry for missunderstanding =)
IsDynamic traverses a given ast to look for a Dynamic ast. But in your case that was a missing unliftables which actually tightly bound to macros itself and #191 confirms this - e.g. there's no way to test this (programmatically at least).

@deusaquilus deusaquilus changed the title Fix for Ad-Hoc Case Class producing Dynamic Queries [WIP] Fix for Ad-Hoc Case Class producing Dynamic Queries Dec 21, 2017
@deusaquilus
Copy link
Collaborator Author

Okay. Should I remove the test then?

@deusaquilus
Copy link
Collaborator Author

Well, you could theoretically call scala.tools.nsc.Main from SBT invoking a particular cp e.g:

-cp io.getquill.MyTest quill-sql/src/main/scala/io/getquill/MyTest.scala

... and then if you can somehow suck-in the compiler output you could check that the query string is present (or check that 'Dynamic Query' is not present). It would be more of an integration test then a unit test though.

@deusaquilus deusaquilus changed the title [WIP] Fix for Ad-Hoc Case Class producing Dynamic Queries Fix for Ad-Hoc Case Class producing Dynamic Queries Dec 21, 2017
@mosyp
Copy link
Collaborator

mosyp commented Dec 21, 2017

@deusaquilus good point on that sbt-specific approach!
Regarding PR, yes pelase remove this test

@fwbrasil
Copy link
Collaborator

I love the number of this PR! 💯0

screen shot 2017-12-21 at 10 55 07 am

@fwbrasil fwbrasil merged commit 7a26f3d into zio:master Dec 21, 2017
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