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

Using JtsSpatialContextFactory.useJtsMulti to control narrowing of collections. #131

Merged
merged 1 commit into from
Jan 28, 2016

Conversation

jdeolive
Copy link
Contributor

This removes the need for custom JtsPolyshapeReader.

@jdeolive
Copy link
Contributor Author

A couple of additional comments.

  • Had to increase the visibility of ShapeFactoryImpl.GeneralShapeMultiShapeBuilder to protected in order to subclass it in JtsShapeFactory
  • Had to update WktShapeParserTest to set useJtsMulti to pass in order to the test case to pass as is

@codecov-io
Copy link

Current coverage is 75.46%

Merging #131 into ShapeFactory will increase coverage by +0.30% as of 88d87f7

@@            ShapeFactory    #131   diff @@
============================================
  Files                 49      48     -1
  Stmts               3419    3420     +1
  Branches             866     867     +1
  Methods                0       0       
============================================
+ Hit                 2570    2581    +11
+ Partial              257     254     -3
+ Missed               592     585     -7

Review entire Coverage Diff as of 88d87f7


Uncovered Suggestions

  1. +0.39% via ...o/GeoJSONWriter.java#79...91
  2. +0.31% via ...ShapeCollection.java#192...201
  3. +0.28% via ...e/io/ParseUtils.java#51...59
  4. See 7 more...

Powered by Codecov. Updated on successful CI builds.

@dsmiley
Copy link
Contributor

dsmiley commented Jan 28, 2016

I added a few comments. Nice work overall (esp. nice to see the test); I'm looking forward to merging it.

@jdeolive
Copy link
Contributor Author

Thanks David. Updated the pull request based on your feedback. Let me know if you want me to squash the commits or if there is anything else that looks off.

@dsmiley
Copy link
Contributor

dsmiley commented Jan 28, 2016

Awesome. Squash & merge!

(I am personally not too familiar with the squashing of feature branches but it's all your changes (thus a squash wouldn't conceal other's work) and it could very well be one commit so might as well have it be.)

…ometry collection.

This removes the need for custom JtsPolyshapeReader.
@jdeolive
Copy link
Contributor Author

Cool. Yeah I tend to prefer squashing feature branches into a single commit as long as (a) a single commit makes sense and (b) it doesn't "hide" any other authors work.

jdeolive added a commit that referenced this pull request Jan 28, 2016
Using JtsSpatialContextFactory.useJtsMulti to control narrowing of collections.
@jdeolive jdeolive merged commit 514c50d into locationtech:ShapeFactory Jan 28, 2016
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.

None yet

3 participants