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

Added support for latest jOOQ version #91

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added support for latest jOOQ version #91

wants to merge 1 commit into from

Conversation

roded
Copy link
Contributor

@roded roded commented Dec 29, 2016

Hi,
Here's a start.
I'm not sure about "comsat-jooq-latest", perhaps "comsat-jooq-java8" is more appropriate.
Please let me know of any comments or reservations.

@pron
Copy link
Contributor

pron commented Dec 29, 2016

Thank you! This looks good, but please change jooq-latest to jooq-java8 as you suggested. Also, take a look at the Quasar build file to see how to exclude subprojects from the build when built using Java 7.

@roded
Copy link
Contributor Author

roded commented Dec 29, 2016

No problem. Could you be more explicit regarding the subproject build exclusion?
Thanks

@roded
Copy link
Contributor Author

roded commented Jan 4, 2017

On my machine, the exclusion of the tests from the sourceSet deletes the test class for some reason, not sure why..

@roded
Copy link
Contributor Author

roded commented Jan 15, 2017

@pron Could you review the gradle solution to make sure it's appropriate?
Anything else I can do to further this along?
Thanks

Copy link
Member

@circlespainter circlespainter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a very, very minor thing but as a convention I personally prefer having all the files ending with a newline, what do you think @pron?

}

assemble.dependsOn jarTask
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sources and javadoc jars are missing.

@@ -495,6 +496,8 @@ The interface now can be registered and used as usual from fibers:
{% include_snippet usage ./comsat-jooq/src/test/java/co/paralleluniverse/fibers/jooq/JooqContextTest.java %}
~~~

As with versions 3.7 and above, jOOQ requires Java 8, Comsat provides two separate jOOQ integrations: The legacy, Java 7 and below, `comsat-jooq` and `comsat-jooq-java8` which supports using jOOQ with Java 8.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change it into:

Starting with versions 3.7 jOOQ requires Java 8 and Comsat provides two separate jOOQ integrations: comsat-jooq supports Java 6+ and jOOQ 3.6.x while comsat-jooq-java8 supports jOOQ 3.8.x with Java 8.

@roded
Copy link
Contributor Author

roded commented Jan 19, 2017

The sources & javadocs artifacts are inherited from the parent build.gradle it seems. Added them explicitly.

@roded
Copy link
Contributor Author

roded commented Jan 24, 2017

The comsat-jooq-java8 build and tests seem OK. The reported build error is a test in the okhttp integration.
A previous build for the same PR (with no changes to the PR code) failed in WebActorServletTest.
I'm not sure if these are related to this PR.

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