-
Notifications
You must be signed in to change notification settings - Fork 46
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
[Java] Refactor Java cookbooks to switch from JShell to Maven #350
Conversation
800c640
to
aa8c53a
Compare
Honestly, why try to support writing Java code inline in reST in the first place? It's a bad experience for recipe authors. If the examples were all external Java files that we could compile there would perhaps be some more boilerplate but it would be nothing unusual for Java and it would avoid the cookbooks being such special snowflakes. |
The current CI failures are the expected ones (one is to bump Avro and the other is the explicit main class thing mentioned above). |
That would probably be better in many ways, though I don't think it solves the problem of how to mix the prose and code together. Might be worth some research into what's already been built that does something like what we're aiming to do here (literate programming) for Java. |
https://github.com/apache/arrow-adbc/blob/main/docs/source/ext/adbc_cookbook.py is fully general, though I haven't actually tried it with Java (but it works with C++ and Python). There's also https://github.com/apache/arrow-adbc/blob/main/docs/source/ext/javadoc_inventory.py which allows crosslinking from Sphinx into Javadoc without hardcoding URLs. |
Thanks @lidavidm, those look like solid options. For the purposes of this PR and getting the cookbooks working for 16.X, I think I'll just wrap the current examples in class defs and plan to refactor things in the near future. |
I updated the Substrait cookbook code and how javadoctest.py works so all the cookbooks pass now locally when I bump Arrow to 16.0.0. I left the version bump out of this PR since we bump versions separately so CI will still fail on the Avro cookbook. I'll file an issue to rework the cookbooks in a sec here. @raulcd do you want to take a look before I merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add some kind of sphinx plugin to insert Java examples from code files into the rst instead of testing the prose. Or even some templating with Jinja to generate the final rst. This probably would make much easier testing the examples.
As for the current code changes, I am not entirely sure about the test failures but if we are happy to merge now and fix those once we upgrade to the new Arrow version I am ok with that.
In #347 we found the way we have been running cookbooks for Java (JShell) doesn't work well with JPMS which was introduced in Arrow 16. This refactors
javadoctest.py
to run examples directly with Maven usingexec:java
instead of with JShell. This PR also bumps the Java source/target version to 11 to fix some compiler errors and fixes a few compilation errors in cookbook code.I ran into one snag will require a follow-up commit to this PR: The way the examples in substrait.rst are written doesn't work with my approach. My approach splits each code snippet into its
import
statements and non-import
statements, puts the imports outside the main class definition and puts the non-imports inside the class's main method. This works fine for every example except substrait.rst which needs some of its code to be defined in the main class, e.g.,We probably generally want to support examples that need this so I think we may need to rewrite all the Java cookbooks to have an explicit main class. @lidavidm suspected this might be the case in #347 (comment) but I do wonder if there is still a way to avoid this. Any ideas welcome.
Fixes #347
Related #348