The idea would be to let Migrate to Java 21 | OpenRewrite Docs do the heavy lifting. It works using “Recipe” which can be group of smaller “Recipes”. We could create a dedicated branch and push a commit for each unitary “Recipe” performed and validated.
Feel free to add any suggestions or say what you think about this approach.
If you feel confident with using this tool, go ahead.
I know IntelliJ can already do a lot (run Java langage migration help as code inspection).
After records, one of the big thing is the switches that are much better with 21 w.r.t. 8.
And of course the getFirst and getLast on List.
Anyway, I’ve tried and failed miserably at locally creating a branch running with JDK 21. IntelliJ does not want to compile it. So if you want to do that for issue 1805, please do.
Actually there is another thing we need to talk about. It’s var. I find it very useful to have the code “breathe” a little bit.
I think we should at least use it when declaring variables, as it’s readily available what class the instance is. For Field stuff in particular, it would avoid the long diamond operators.
What do you think?
I do agree with this, especially with Field as you mentioned. Java is already verbose enough ! This could be a dedicated commit and i think there is already a Recipe for this in OpenRewrite.
It crossed my mind that we should have the Orekit tutorials and performance run first in java 21 otherwise the library itself is going to have red pipelines when we switch
I’m (re)starting the migration process to java 21, i’m rebasing the current migation branch on develop because too many changes happened since first experiment (as expected).
Ok, so orekit-tutorials and orekit-performances have been upgraded to Java 21. I’m now getting a SonarQube failure due to insufficient coverage on Orekit (93.5% instead of the required 95%), mainly on condition coverage. This might be related to SonarQube behavior rather than an actual regression.
I did a quick spot check and the changes look fine to me. It will take me a bit to get used to the new instanceof and switch statements.
Some of the code had been formatted to look like a table, e.g. UTCScale, and so using getFirst() instead of get(0) makes that line a bit longer. Not a big deal.
Looks like the code coverage issues are legitimate functionality that was not covered by existing unit tests. I’m guessing you’re hitting the Sonar gate because a high percentage of your changes are conditionals, like instanceof and switch.
Yes i did not want to force a specific format yet, it can be done in another merge request with a format we agree on.
That is also my thought, i’m wondering if we should merge with current state and then add tests or if we should add more unit tests in this MR specifically.
I think you should merge, unless you think it’s fast to add the tests, tell us.
We can temporarily downgrade the Sonar gate to 93.5% and add tests afterwards to reach the 95% mark
I can take a look tonight to see if i can add some tests easily but i do think merging this before other merge requests are approved is the best option. Otherwise it will be a neverending nightmare
For what it’s worth, when some code uses both get/add(0), get/add(1) and so on, I don’t really think replacing the (0) part with First is relevant. In my humble opinion, it is worth doing only when we only retrieve the first element (or the last one, of course).
Tonight ? Don’t have time to do this during your workday? That’s too bad, because this migration isn’t just a nice-to-have. It’s really going to benefit everyone.
