Skip to content

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Nov 10, 2024

This change makes the lat and lon fields optional (see discussion in #112), but adds some validation logic:

  • The location fields needs at least one property
  • If lat is specified, then lon is required as well (and vice versa)

This means that "location": {}, is not valid (at least 1 property required), neither is "location": {"lat": 1.23} (both lat and lon) must be specified). But "location": {"timezone": "Antarctica/Palmer"} is valid.

Logic is based on the latest JSON Schema validation draft. I don't know whether there is significant support in tooling for these fields.

Fixes #112

@dbrgn dbrgn requested review from gidsi, rnestler and s3lph November 10, 2024 17:24
@dbrgn dbrgn force-pushed the 112-lat-lon-optional branch 3 times, most recently from f3ade43 to e1e318d Compare November 10, 2024 17:28
@dbrgn dbrgn force-pushed the 112-lat-lon-optional branch 2 times, most recently from cf12fed to 1d32216 Compare November 10, 2024 18:54
- The `location` fields needs at least one property
- If `lat` is specified, then `lon` is required as well (and vice versa)
Copy link
Member

@s3lph s3lph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dbrgn dbrgn merged commit 20e924b into master Nov 10, 2024
8 checks passed
@dbrgn dbrgn deleted the 112-lat-lon-optional branch November 10, 2024 22:59
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.

Location: Should lat/lon be optional as well?

3 participants