-
Notifications
You must be signed in to change notification settings - Fork 232
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
Implementing JSON Schema for Provider #53
Implementing JSON Schema for Provider #53
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome @thekaveman. I know you are into the versioning of these specs: what do you think about being even stricter with the version field?
I think it's a good idea to include the schemas for the GeoJSON componenents. The good news is that the definers of the format have provided schemas. Here is code to auto-generate them. The README links to the actual schemas. As far as I can tell, the JSON schema spec allows you to reference other schemas by URI, so it should be relatively straightforward to link to those. For example, here is the schema for Point
:
http://geojson.org/schema/Point.json
provider/schema.json
Outdated
"examples": [ | ||
"0.1.0" | ||
], | ||
"pattern": "^(.*)$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this pattern can be even stronger than this. We know that this schema is 0.1.0
, not just any string. We can also expect various non-breaking changes that result in patches to this schema, and therefore could change the third number. So the pattern could be something stricter like
^0\.1\.[0-9]+$
Thereby enforcing the major version number, and letting the minor version be any number.
Alternatively, we could store the major and minor version numbers as number
s, rather than strings, and have something like
"majorVersion": {
"description": "Major version number",
"type": "integer",
"minimum": 0,
"maximum": 0
},
"minorVersion": {
"description": "Minor version number",
"type": "integer",
"minimum": 1,
"maximum": 1
},
"patchVersion": {
"description": "Patch version number",
"type": "integer",
"minimum": 0
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I just left the default there but SemVer is well-defined enough that we could easily tighten it.
I don't know about hard-coding the minorVersion
in a pattern like ^0\.1\.[0-9]+$
, but maybe with some kind of generation tool it wouldn't be that big of a deal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that the version of the schema could be incremented with each publishing of a new version. The publisher would decide: "is this a bugfix, a backwards compatible change, or a breaking change?", and then change the pattern (or integer field) accordingly.
provider/schema.json
Outdated
"examples": [ | ||
"3c960026-b5ee-11e8-96f8-529269fb1459" | ||
], | ||
"pattern": "^([0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12})$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Boy, the UUID pattern is not messing around, is it?
provider/schema.json
Outdated
} | ||
} | ||
}, | ||
"status_changes": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding of the narrative description is that the JSON payloads are different between whether they hit the trips
or status_changes
endpoints. So having the JSON schema describe both of them at once seems like it is not the right thing. Instead, I would think that there would be a separate schema for each of the endpoints (maybe both deriving from a common base schema?).
Or am I misunderstanding things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's one of the things I meant to put on my list above... definitely needs to be cleaned up.
Reading about combining schemas got me thinking that something along those line might work. Just haven't thought about how to do it yet.
Ah, I see some of my comments were also addressed in your initial statement, sorry. |
@thekaveman @ian-r-rose status on this? Would love to merge into |
I think CityofSantaMonica#1 is close to ready. @thekaveman and I were uncertain about whether it was best to include some of the tooling (validation + schema generation scripts) in this repo, or an associated one. Also happy to open that PR against this repo to supersede this one. |
@hunterowens I thought this was good for Outstanding concerns to address for this to be ready:
|
Also, are you using |
@ian-r-rose going forward that is the case. I just changed this PR to be against |
I'm going to rebase this on |
common definitions.
3ed4bbd
to
8b792d1
Compare
Ok, I think we still need this PR to address the following:
(I'm going to go through it again and potentially add to this list, feel free to do so anyone else @hunterowens @ian-r-rose) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating @thekaveman, these changes look good to me. Just one future-proofing thought.
generalized the generator code a bit to handle other situations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌮 💯 💯 🥇 to @thekaveman and @ian-r-rose
The following commit implements JSON Schema for Provider. Agency TK * Use official FeatureCollection schema. * Use custom FeatureCollection spec. * Add semver-aware version. * Move propulsion_type into a common definition. * Add vehicle_type definition. * Make event_position a `Point`. Reuse the `Point` schema. * Test schema. * Fix name of schema. * Auto-getnerate schemas without nonlocal refs. * Add some docstrings. * Rename directory. * Inadvertantly added this file. * generate and reference a FeatureMDS definition
This reverts commit 71412b5.
The following commit implements JSON Schema for Provider. Agency TK * Use official FeatureCollection schema. * Use custom FeatureCollection spec. * Add semver-aware version. * Move propulsion_type into a common definition. * Add vehicle_type definition. * Make event_position a `Point`. Reuse the `Point` schema. * Test schema. * Fix name of schema. * Auto-getnerate schemas without nonlocal refs. * Add some docstrings. * Rename directory. * Inadvertantly added this file. * generate and reference a FeatureMDS definition
This is for #46 and more of a test of the idea than fully-baked implementation.
There is plenty of room for improvement:
Define the top-level envelope structure once:
and re-use it across different parts of the overall MDS schema.
Define the common objects (
route
,vehicle_type
,propulsion_type
, etc.) once and referenceReference (existing?) geoJSON schemas if applicable
I'm sure there are others.