Add missing id, name fields to json schema docs

hellosmithy commented 4 days ago

Added the optional id and name fields which are documented on the FINOS site but currently missing from OrderList, InstrumentList, ContactList and Contact (name only) json schema docs.

See:

linux-foundation-easycla[bot] commented 4 days ago

:x: - login: @hellosmithy / name: Ben Smith . The commit (8ef6005bcdca4caa0db6598392f603696f1a9af6, 64c5e7803144be4814cc2a637e8769d7778759eb) is not authorized under a signed CLA. Please click here to be authorized. For further assistance with EasyCLA, please submit a support request ticket.

netlify[bot] commented 4 days ago

Deploy Preview for fdc3 ready!

Name	Link
Latest commit	8ef6005bcdca4caa0db6598392f603696f1a9af6
Latest deploy log	https://app.netlify.com/sites/fdc3/deploys/66821858f00904000803c5e5
Deploy Preview	https://deploy-preview-1233--fdc3.netlify.app
Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

kriswest commented 3 days ago

Hi @hellosmithy, technically the id and name properties are 'inherited' from the base Context schema, which is composed with each of these via allOf: https://github.com/finos/FDC3/blob/13ccdd2b7b57aa81721ee136e54f42e27e65cf8c/schemas/context/contact.schema.json#L41C28-L41C28

which end up in the TypeScript types generated from the resulting composed schema: https://github.com/finos/FDC3/blob/dcd98c2111d4d815ddf7d5cd3f4d91c536be414a/src/context/ContextTypes.ts#L1247-L1266

However, it's not really inheritance (composition of schemas is more like layering filters over one another) and there is no problem defining them in multiple schemas that get composed together. You'll note we use the BaseContext without docs, allowing the downstream schemas to add their own without doubling up - which your PR does.

Finally, a note for the maintainers, the preview for PR #1151 (generate Context docs from schemas) shows that the docs generation script is not picking up the composed schema refs and rendering the Context schema properties: https://issue-1068--exquisite-otter-12d363.netlify.app/docs/context/schemas/Contact We can either fix that (not trivial) or continue down this path and document the id and name properties where they make sense or are more tightly defined (e.g. specific id subfields). This latter option is simpler and adds more value to the docs / avoids rendering name and id in context docs where they are not so relevant.

kriswest commented 3 days ago

@hellosmithy Once you've got a CLA manger setup, please click on the Please click here to be authorized link above to have EasyCLA check it and add you to its database of approved contributors, which will allow us to merge this once reviewed.

hellosmithy commented 3 days ago

technically the id and name properties are 'inherited' from the base Context schema, which is composed with each of these via allOf

Ah yes, I knew about the composition but didn't realise it included id and name. ~~For some reason the json-schema-to-typescript npm package I'm using to generate some types also doesn't appear to pick these up which is what brought my attention to it.~~ (nvm, I was wrong about that)

hellosmithy commented 3 days ago

Slight tangent, but are type fields supposed to be case sensitive or insensitive? TimeRange has camel case timeRange in the example, but lowercase timerange in the schema type field.

timeRange as per the examples seems more semantically correct, but I guess that might be a breaking change so it might be easier to change the example.

kriswest commented 2 days ago

Ah yes, I knew about the composition but didn't realise it included id and name. For some reason the json-schema-to-typescript npm package I'm using to generate some types also doesn't appear to pick these up which is what brought my attention to it.

If I remember rightly, json-schema-to-typescript doesn't retrieve references ($ref). You can can do that yourself by just replacing the $ref element with the referenced schema. https://www.npmjs.com/package/json-schema-resolver I think can do that for you as well.

In the FDC3 repo we use quicktype (which possibly combines these two libs under-the-hood) to generate typescript types from the schemas.

kriswest commented 2 days ago

Slight tangent, but are type fields supposed to be case sensitive or insensitive? TimeRangehttps://fdc3.finos.org/schemas/2.1/context/timerange.schema.json has camel case in the example, but lowercase in the schema

They are case-sensitive, so the example there is wrong. I'll get that fixed. Many thanks for pointing it out!

kriswest commented 2 days ago

@hellosmithy could you run npm run typegen or npm run build for me and check in the changes it makes? This will regenerate the typescript files produced from the schema and ensure they make it into the diff.

I should probably have this added to the pre-commit hook so that it's automated!

finos / FDC3