fix(csvjson): a blank --lat/--lon crashes instead of writing a null geometry - #1355
Open
VXNCXNX wants to merge 1 commit into
Open
fix(csvjson): a blank --lat/--lon crashes instead of writing a null geometry#1355VXNCXNX wants to merge 1 commit into
VXNCXNX wants to merge 1 commit into
Conversation
Catch TypeError in addition to ValueError when parsing coordinates, guard geometry membership test with feature.get(), and omit bbox when no coordinates are present. Fixes errors on rows with blank/unparseable lat/lon values.
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.
What's broken
csvjson --lat/--lonaborts on any row whose coordinates are blank.A blank coordinate is the ordinary case in a geocoded export: one address failed to resolve and the rest are fine. One such row loses the whole file.
After:
"geometry": nullis what RFC 7946 says an unlocated feature is, so the row is kept and marked rather than dropped.Three defects on one seam
float(None)raisesTypeError, notValueError. The existingexcept ValueErrorwas already there to tolerate a bad coordinate, and it does catchfloat("abc"). But an empty cell arrives asNone, not"", so it takes a different exception type and escapes the handler that exists to deal with exactly this.'coordinates' in feature['geometry']runs against aNone.geometry_for_rowreturnsNoneimplicitly for a row with no coordinates, so the membership test raisesTypeError: argument of type 'NoneType' is not iterable.--no-bboxskips this path entirely, which is why the shape can look like it works.A bbox of nulls is not valid GeoJSON. With no usable coordinates anywhere, the output was
"bbox": [null, null, null, null]. RFC 7946 requires bbox members to be numbers, and bbox itself is optional, so the right answer is to omit it.The fix
Widen the existing catch to
(TypeError, ValueError), guard the bbox accumulator withfeature.get('geometry'), and add anis_set()check so the bbox key is only emitted when at least one coordinate was seen.Rows with valid coordinates are unaffected, and so is
--no-bbox.Verification
Two cases in
tests/test_utilities/test_csvjson.pywith two small fixtures, one file mixing a good row with a blank one, one where every row is blank. They assert the bbox still covers the good row, that the blank row's geometry isnull, and that no bbox key appears when nothing was located.Reverting all three hunks fails both with the original
TypeError: float() argument must be a string or a real number, not 'NoneType'. Reverting only the bbox hunk fails with'bbox' unexpectedly found in {... 'bbox': [None, None, None, None] ...}, so each hunk is pinned separately.pytest tests/test_utilities/test_csvjson.pyis 26 passed. The full suite is4 failed, 349 passed, and the same 4 csvstat locale failures occur on a clean tree here.