SpaceApi / schema

SpaceAPI JSON schema files.
24 stars 14 forks source link

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

Open dbrgn opened 2 months ago

dbrgn commented 2 months ago

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.

rnestler commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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).