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

feat: Migrate ApplicationType to alternative enum structure #218

Merged
merged 11 commits into from
Jul 31, 2024

Conversation

DafyddLlyr
Copy link
Contributor

@DafyddLlyr DafyddLlyr commented Jul 26, 2024

Frustratingly JSONSchema doesn't support description annotations per-enum (which is very likely what lead us to this more complex structure we currently use).

You can see in this PR enums with @description notation (PA, PP) not being pulled through into the generated schema.

For WTT, I've tried adding the descriptions as JSON within the @description block. This works and allows us to encode this information in the schema, but it's not typesafe or particularly consumer friendly. It might be a decent compromise though? Thoughts very welcome!

Update: The above is now outdated, please see #218 (comment) - I have a solution which works well, we'll go with this going forward.

@DafyddLlyr DafyddLlyr changed the title feat(wip): Different enum structure feat(wip): Alternative enum structure Jul 26, 2024
Base automatically changed from dp/generic-application-type to main July 29, 2024 11:24
Comment on lines 437 to 439
/**
* @description Rights of Way Order - Apply to move or close a path
*/
Copy link
Contributor Author

@DafyddLlyr DafyddLlyr Jul 29, 2024

Choose a reason for hiding this comment

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

This method doesn't work (@description per-enum)...

Comment on lines 442 to 449
/**
* @description
* {
* "wtt": "Works to trees",
* "wtt.consent": "Consent to carry out works to a tree with a Tree Preservation Order",
* "wtt.notice": "Notification of proposed works to a tree in a Conservation Area"
* }
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

...but this does (JSON in @description of parent).

@jessicamcinchak
Copy link
Member

jessicamcinchak commented Jul 29, 2024

Have been doing some reading on enums in JSON Schema this morning & definitely seems like simple enum types come with the usability trade off/limitation of no support for @description annotations - and lots of things point towards the oneOf/anyOf dict style we have now as the "best practice" alternative (trade offs of harder to parse validation errors regularly noted).

But I did find one option that I quite like - this meta:enum key that's part of jsonschema2md mentioned here https://stackoverflow.com/questions/64233370/in-json-schema-how-to-define-an-enum-with-description-of-each-elements-in-the-e

Since we know some type of improved public documentation is also on our near-roadmap here, maybe this could be a two-for-one? I suspect we'd have to globally set "additionalProperties": true to support this and extend the ts to json schema generator script ourselves to pick this up (both of which may be too large of efforts to justify!), but just wanted to flag as it feels like exactly what we want !

@DafyddLlyr
Copy link
Contributor Author

DafyddLlyr commented Jul 29, 2024

Thanks @jessicamcinchak - that link was super helpful, and lead to me this thread which suggested a combination of anyOf and const.

Take a look at aa7f59a - this looks like the solution we're looking for? 🤞

It looks like ts-json-schema-generator will combine plain string unions, but convert named types to a list of anyOf + consts.

@DafyddLlyr DafyddLlyr changed the title feat(wip): Alternative enum structure feat: Migrate ApplicationType to alternative enum structure Jul 31, 2024
@DafyddLlyr DafyddLlyr requested a review from a team July 31, 2024 08:55
Copy link
Member

@jessicamcinchak jessicamcinchak left a comment

Choose a reason for hiding this comment

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

Very happy with the latest ApplicationType changes - let's merge this then we can work through other enums 🙂

@DafyddLlyr DafyddLlyr merged commit f7dd380 into main Jul 31, 2024
3 checks passed
@DafyddLlyr DafyddLlyr deleted the dp/simple-enums branch July 31, 2024 09:55
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.

2 participants