Skip to content
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

Location: Should lat/lon be optional as well? #112

Closed
dbrgn opened this issue Apr 14, 2024 · 7 comments · Fixed by #115
Closed

Location: Should lat/lon be optional as well? #112

dbrgn opened this issue Apr 14, 2024 · 7 comments · Fixed by #115
Milestone

Comments

@dbrgn
Copy link
Contributor

dbrgn commented Apr 14, 2024

In #106 we made the top-level location field optional. If it is defined, the lat/lon fields still need to be defined though.

Some virtual spaces might not have a location, but they might still have:

Maybe we should make the location required again, but allow omitting all fields?

"location": {}

Alternatively, we could keep location as optional, but make location.lat and location.lon optional as well. (Potentially with some rules to say that if one of them is defined, the other one must be defined as well.)

Thoughts, @SpaceApi/core? This question is a release blocker for v15.

@dbrgn dbrgn added this to the API v15 milestone Apr 14, 2024
@rnestler
Copy link
Member

Maybe we should make the location required again, but allow omitting all fields?

I don't think this provides much value. What's the purpose of an empty object?

Alternatively, we could keep location as optional, but make location.lat and location.lon optional as well. (Potentially with some rules to say that if one of them is defined, the other one must be defined as well.)

I'd like that.

This question is a release blocker for v15.

I don't think it should really be a blocker.

@dbrgn
Copy link
Contributor Author

dbrgn commented May 20, 2024

I don't think this provides much value. What's the purpose of an empty object?

The empty object isn't the goal in itself, but it would be an alternative form of indicating that a space has no location.

The suggestion with a required "location" would mainly be that there aren't two ways to communicate the same thing. If the location can be omitted, and lat/lon are optional as well, then these two variants are equivalent in meaning:

{
  "location": {}, // Legal, because lat/lon are optional
}
{
  // Omit location completely
}

With the discussion about null potentially being an allowed value, there would even be a third way to indicate the same thing (although we don't currently allow null anywhere in the v15 draft):

{
  "location": null,
}

I generally prefer to have one right (canonical) way to represent something.

Keep in mind that in v15 we're explicitly making the location key optional. The first suggestion would be to instead make the lat/lon fields optional. That's also why I marked it as release blocker, because the discussion is about potentially reverting a decision we did in the current v15 draft. (And such things are easier before the release.)

@rnestler
Copy link
Member

My favored solution would be to have location optional but require that if it is provided that at least one subkey must be provided (so making "location": {} illegal).

@dbrgn
Copy link
Contributor Author

dbrgn commented May 20, 2024

My favored solution would be to have location optional but require that if it is provided that at least one subkey must be provided (so making "location": {} illegal).

Can this be easily implemented? Sounds like quite complex validation logic to me, that probably not every consumer of the JSON schema will implement 🙂

But to keep things simple, we can simply make both lat/lon optional and keep the location key optional as well (because I also think that an empty object is a bit ugly). The vast majority of spaces will have a location anyways.

@rnestler
Copy link
Member

I'd suggest to leave it as is for v15 (optional location field, but if it is present have at least lat/lon) to not block the release.

But to keep things simple, we can simply make both lat/lon optional and keep the location key optional as well (because I also think that an empty object is a bit ugly). The vast majority of spaces will have a location anyways.

This can then be implemented in a v16 and we could add validation that if lat is present also lon is present or similar (should be doable with anyOf).

@s3lph
Copy link
Contributor

s3lph commented Nov 10, 2024

I think making both the top-level location field and lat/lon optional would cover the most use cases while not introducing an empty "location": {}, so I'd agree with #112 (comment) the most.

Another thing that comes to mind: Can we specify that of one of lat and lon is provided, the other one must be provided as well? Otherwise having only lat or only lon, but not the other would be valid.

@dbrgn
Copy link
Contributor Author

dbrgn commented Nov 10, 2024

I think I found a great solution: See #115

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 a pull request may close this issue.

3 participants