locationtech / spatial4j

LocationTech Spatial4j: A Geospatial Library for Java
https://projects.eclipse.org/projects/locationtech.spatial4j
932 stars 167 forks source link

Upgrade to JTS 1.17.0. #188

Closed jnh5y closed 4 years ago

jnh5y commented 4 years ago

Signed-off-by: Jim Hughes jnh5y@ccri.com

dsmiley commented 4 years ago

Thanks! Can you update CHANGES.md as well please? In like-kind to past entries.

jnh5y commented 4 years ago

@dsmiley of course. Are the numbers, the PR number or the issue number? (I assumed the former.)

Any more that I can say for the update?

codecov-commenter commented 4 years ago

Codecov Report

Merging #188 into master will decrease coverage by 0.07%. The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #188      +/-   ##
============================================
- Coverage     75.84%   75.76%   -0.08%     
+ Complexity     1108     1105       -3     
============================================
  Files            57       57              
  Lines          3817     3817              
  Branches        776      774       -2     
============================================
- Hits           2895     2892       -3     
  Misses          643      643              
- Partials        279      282       +3     
Impacted Files Coverage Δ Complexity Δ
...iontech/spatial4j/shape/impl/ShapeFactoryImpl.java 79.12% <0.00%> (-2.20%) 37.00% <0.00%> (-2.00%)
.../locationtech/spatial4j/shape/jts/JtsGeometry.java 77.41% <0.00%> (-0.36%) 79.00% <0.00%> (-1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 63b3183...d3f6d9a. Read the comment docs.

dsmiley commented 4 years ago

JTS 1.17 included a change to upgrade Java 1.7 to 1.8 -- https://github.com/locationtech/jts/releases/tag/1.17.0 and this means Spatial4j's builds must now require at least 1.8 as well, but it can keep javac source & target options to 1.7. Users can continue to run Spatial4j with Java 1.7 but will need to upgrade to use the JTS we specify if they want to use JTS (it's an optional dependency). Strangely, TravisCl hasn't been pinging me about the failure but I did seek it out: https://travis-ci.org/github/locationtech/spatial4j/builds/706041451 I wish it showed up here in the PR. Hmm; something to look in to.

jnh5y commented 4 years ago

Sorry for that oversight on my part. I didn't realized that Spatial4J was still supporting Java 7 and that JTS had moved up to requiring Java 8 in 1.17.0.

I don't see the results from the Git WebHook with Travis CI here. Maybe those need to configured differently? I know for GeoMesa that the GitHub webhooks will show green check or red X's for a number of criteria. (Let me know if I can help with any of that.)

dsmiley commented 4 years ago

Yes please, I would appreciate help hooking up Travis CI to the PR. Do I just need to RTFM (URL please)?

jnh5y commented 4 years ago

We'll likely have to email the Eclipse webmaster to ask about some GitHub settings.

After thinking about it, would it be worth considering GitHub Actions instead? (If you are willing to try them, I may be able to find a few minutes to gin up an example of GH Actions that'd run the builds you are using Travis for.)

If we go that route, is Java 7 support still important to you? (If so, that's fine. That said, I'd love to hear a reason why.)

dsmiley commented 4 years ago

No Java 7 is too old, but I still -source & -target at the complication step to 7. Note that since I added in JDK 11 to Travis very recently, there seems to be some test maintenance for me to do in order to make tests work in JDK 11. Work for me to do soon.