Finish the NewEnvelopeXY constructor - #711
Merged
Merged
Conversation
NewEnvelopeXY landed early as part of the test helper consolidation in #707, without the finishing that the rest of the XY constructor family has. Move it out of the middle of the Polygon block, where it separated NewPolygonXYZM from NewSingleRingPolygonXY, to sit after the geometry type constructors. Document that the result is the smallest Envelope containing all of the coordinates, which the variadic signature doesn't convey, and add the note about validation that every sibling constructor carries. Envelope has a Validate method, so the note applies here too. Closes #710
The ClipByRect test cases spelled out two XY literals with named fields for each rectangle, which is the verbosity NewEnvelopeXY exists to avoid. The TWKB bbox header and the zig zag ring benchmark had already computed their bounds as float64s, so they were wrapping them in XY values only to hand them to NewEnvelope. Call sites that hold XY values already, such as the type_envelope tests that exercise NewEnvelope itself, are left alone.
"Joins the other XY constructors" could be read as joining or combining data, which is a plausible meaning in a geometry library. Say that the constructor follows the same convention instead.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
NewEnvelopeXYis already on master. It landed as part of the test helperconsolidation in #707, which needed it for the envelope tests, and it went in
without the finishing that the rest of the
XYconstructor family has. So thisPR completes #710 by tidying up the existing function rather than adding it.
Two things about the constructor itself:
It sat between
NewPolygonXYZMandNewSingleRingPolygonXY, separating thetwo halves of the Polygon block. It now sits after the geometry type
constructors, since an
Envelopeisn't one of them.Its doc comment didn't mention that the result is the smallest
Envelopecontaining all of the coordinates, which the
xys ...float64signaturedoesn't convey. It was also missing the note about validation that every
sibling constructor carries.
Envelopehas aValidatemethod, so the noteapplies here too.
The second commit uses the constructor where envelopes were being built from
literals:
The
ClipByRecttest cases each spelled out twoXYliterals with namedfields per rectangle, which is the verbosity
NewEnvelopeXYexists to avoid.The TWKB bbox header and the zig zag ring benchmark had already computed their
bounds as
float64s, so they were wrapping them inXYvalues only to handthem to
NewEnvelope.Call sites that hold
XYvalues already are left alone, including thetype_envelopetests that exerciseNewEnvelopeitself.Check List
Have you:
Added unit tests?
TestNewEnvelopeXYcame in with Consolidate test helpers into internal/test package #707 and covers theno-argument, single pair, two point, reversed order, three point, and odd
count panic cases. This change needed no new cases.
Add cmprefimpl tests? (if appropriate?) Not appropriate. There is no PostGIS
or GEOS counterpart to compare a Go constructor against.
Updated release notes? (if appropriate?) Yes. The existing
Unreleasedentryis expanded to describe the coordinate ordering, the empty and point envelope
cases, and the panic.
Updated the README.md (if new functionality is added?) Not appropriate. The
README doesn't enumerate constructors.
Related Issue
Closes Add a NewEnvelopeXY constructor #710.
Make polygon construction easier #477 listed "Include
Envelopeconstructors as well" among the helpers toadd.
Benchmark Results
Not applicable, so no benchmark run is included. The
twkb_parser.gochangepasses the same four
float64s toNewEnvelopeXYinstead of packing them intotwo
XYvalues first, and the remaining changes are in test and benchmarksetup.