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

GH-162 handle empty point / shape collection in toString #183

Merged

Conversation

abrokenjester
Copy link
Contributor

@abrokenjester abrokenjester commented Mar 9, 2020

Added corner case handling for serializing points and shape collections to string.

Fixes issue GH-162

@codecov-io
Copy link

codecov-io commented Mar 9, 2020

Codecov Report

Merging #183 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #183      +/-   ##
============================================
+ Coverage     75.03%   75.09%   +0.06%     
- Complexity     1092     1095       +3     
============================================
  Files            57       57              
  Lines          3805     3811       +6     
  Branches        772      774       +2     
============================================
+ Hits           2855     2862       +7     
  Misses          671      671              
+ Partials        279      278       -1
Impacted Files Coverage Δ Complexity Δ
.../java/org/locationtech/spatial4j/io/WKTWriter.java 100% <100%> (ø) 19 <0> (+2) ⬆️
...iontech/spatial4j/shape/impl/ShapeFactoryImpl.java 80.21% <0%> (ø) 38% <0%> (ø) ⬇️
.../locationtech/spatial4j/shape/jts/JtsGeometry.java 77.77% <0%> (+0.35%) 80% <0%> (+1%) ⬆️

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 86f83bf...07081e7. Read the comment docs.

@abrokenjester abrokenjester force-pushed the GH-162-wktwriter-emptypoint branch 2 times, most recently from 12f8b11 to 028a36a Compare March 9, 2020 02:42
@dsmiley
Copy link
Contributor

dsmiley commented Mar 9, 2020

Thanks! Please add a CHANGES.md entry and then I look forward to squash-merging this!

@dsmiley
Copy link
Contributor

dsmiley commented Mar 12, 2020

Maybe GeneralWktTest.java would be a better existing place to put this assuming we round-trip and also that this is specific to WKT at this time (GeoJSON empty not yet supported -- I don't know for sure)

hsbakshi and others added 4 commits March 12, 2020 15:08
Details of the problem are explained here: locationtech#177

The new test-case fails without the code change and passes after the code change.

Signed-off-by: Hrishi Bakshi <[email protected]>
Signed-off-by: Jeen Broekstra <[email protected]>
@abrokenjester
Copy link
Contributor Author

I've added a changelog entry - had to force-push as I forgot to sign off at first. Not sure where the conflict comes from as I only added the one line, but I'll leave that in your capable hands :)

@dsmiley
Copy link
Contributor

dsmiley commented Mar 14, 2020

I have some changes that more extensively work across GeoJson as well and testing. I tried to push to your PR but it was rejected. Have you seen this?: https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/allowing-changes-to-a-pull-request-branch-created-from-a-fork
If I can't push to your PR than how about I create a new PR with your change and mine on top? Or I do a PR to your feature branch on your fork?

@abrokenjester
Copy link
Contributor Author

Peculiar - the "allow edits from maintainers" option was already enabled, so you should have been able to push to my branch.

One way to solve this if you create a new feature branch for this change in the main repo, then re-target this PR to that feature branch, merge it, and then do your own additional changes on top. But if you prefer doing a PR on my fork instead that's fine too - whatever works for you :)

@dsmiley
Copy link
Contributor

dsmiley commented Mar 16, 2020

I was mistaken; my initial attempts to push to your branch were actually to a new branch and not to your existing. I corrected that and as you can see they are here now.

Copy link
Contributor Author

@abrokenjester abrokenjester left a comment

Choose a reason for hiding this comment

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

Looks good!

@dsmiley dsmiley merged commit 6a43828 into locationtech:master Mar 20, 2020
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.

4 participants