Skip coordinates that Geo::Coord rejects instead of raising - #105
Open
thatbudakguy wants to merge 1 commit into
Open
thatbudakguy wants to merge 1 commit into
thatbudakguy wants to merge 1 commit into
Conversation
NavPlace#valid? is documented to indicate whether the coordinate texts are
valid, and it returns false for text that does not match COORD_REGEX. But when
text matches the regex and Geo::Coord then rejects the result, the ArgumentError
propagated out of valid?, so a predicate meant to guard against invalid input
raised on it instead. Callers that did the documented thing --
`nav_place.valid? ? nav_place.build : nil` -- still got an exception.
Geo::Coord rejects two things that occur in real MARC coordinate data: degrees
outside the valid range ("N 543°00′00″"), and a hemisphere that does not belong
to the axis it was given for ("E 34°50′" as a latitude). coord_for now returns
nil for both, the same as text that did not match at all, so the coordinate is
skipped, valid? reports false, and build raises its own 'invalid coordinates'
error only when nothing usable is left.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
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.
Why
NavPlace#valid?is documented as "indicates if coordinate_texts passed in are valid", and it returnsfalsefor text that doesn't matchCOORD_REGEX. But when text does match the regex andGeo::Coordthen rejects the result, theArgumentErrorpropagates straight out ofvalid?— so the predicate meant to guard against invalid input raises on it instead.Callers doing exactly what the API invites still get an exception:
This is a live 500 in sul-dlss/purl's IIIF manifest endpoint.
Geo::Coordrejects two things that turn up in real MARC coordinate data, both reaching us from cataloged Stanford records:Geo::CoorderrorW 008°00′00″--W 004°00′00″/N 059°00′00″--N 543°00′00″Expected latitude to be between -90 and 90, 0.543e3 received(W 117°57'--W 117°39'/E 34°50'--E 32°48').Unidentified hemisphere: E— latitudes labeledEThe second is the more interesting shape: the degrees are in range, but the hemisphere belongs to the wrong axis.
What changed
coord_forrescuesArgumentErrorand returnsnil, which is already how the method reports text it can't use (return unless long_matcher && lat_matcher). So an unusable coordinate is skipped by the existing.compact,valid?reportsfalse, andbuildraises its own'invalid coordinates'only when nothing usable is left.Rescuing in
coord_forrather than invalid?matters:build,features, andpolygon_geometryall reachcoordinatestoo, so a narrower fix would still leavebuildable to raise a rawGeo::Coorderror.Verification
#valid?and#build, using the real record values above, plus a mixed case asserting that one unusable coordinate is skipped while the valid ones still build.