-
Notifications
You must be signed in to change notification settings - Fork 8
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
Change coordinate handling logic #9
Conversation
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.
I think this is not actually bad overall, despite the scary X that GitHub puts when I do a "Request Changes" vote (edit: apparently they've changed it since the last review I voted that way, which makes me happy)... see specific points I'd ask to be addressed in some way before merging.
I'd also like to see a response from @FObermaier before I move to merge it, since he had the stronger opinions on that thread (mine were just minor concerns about consistency, one of which even appears to have been unfounded after re-reviewing SqlServerBytesReader
).
Thank you for your submission!
@airbreather pushed commits as per your review comments, but one important question is still open... |
Responded on one of them, but otherwise I'm fine with this once that one question is resolved. Note that I did merge #7 (after you'd originally submitted this, sorry for the conflicts), so I think one of your interim commits (b27ac38) may be redundant. |
* HandleOrdinates on both reader and writer now act only as an "upper bound" - coordinates not specified in HandleOrdinates will be stripped when reading and writing objects. * The meaning of Ordinates.None has been changed to mean that objects are read and written as-is, without either stripping or adding any coordinates. * Ordinates.None is now the default. Fixes NetTopologySuite#8
38d5615
to
af087bb
Compare
@airbreather I rebased on the latest master, incorporating #7 and resolving the conflict. I also squashed all commits - let me know if there's anything else you'd like to have changed or discussed. |
From a brief look, I think it looks great -- I'll double-check tonight. @FObermaier do you have any lingering concerns? If not, I intend to merge it tonight unless my "double-check" reveals a compelling reason not to. |
@airbreather I have no objections to merging the PR. |
...well... turns out Edit: really quick extension method fixed it right up, so I've added a commit on top and merged. Thanks again! |
|
My preference would be to make the NTS libraries |
This PR changes ordinate handling based on the discussion in #8, and as detailed below. @FObermaier I'm not sure if this is exactly what you intended, but please take a look (especially at the new test) to see the proposed behavior; if you guys see an issue with it I can revisit and amend. @YohDeadfall and @austindrenski your input would be great too.
Fixes #8