-
Notifications
You must be signed in to change notification settings - Fork 73
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
Add JSON schema struct generation #210
Conversation
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
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.
Looks good 👍
- I think we should also add a
Makefile
with a rule to regenerate these types. - The docs could be updated to include a reference to this JSON schema in the operations reference section
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
@andrew-farries Updated!
Proposed follow-ups:
|
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
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 looks good, just a few more suggestions.
@@ -0,0 +1,404 @@ | |||
{ | |||
"$schema": "https://json-schema.org/draft/2020-12/schema", | |||
"allOf": [{ "$ref": "#/$defs/PgRollMigration" }], |
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.
What's the purpose of allOf
here? The same types are still generated successfully if I remove this line.
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.
Some tooling fails to parse $ref
in the top level. The allOf
is a workaround that makes it work.
I think we could also move PgRollMigration
definition top level instead of a $ref
, not sure what you'd prefer. It's the only definition we actually don't really need in the types.go
file.
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.
The PgRollMigration
type actually duplicates a type we define here:
https://github.com/xataio/pgroll/blob/main/pkg/migrations/migrations.go#L46-L53
So it could probably be removed entirely.
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 missed those. What about I create: Migration
, Operation
and Operations
definitions and keep the allOf
with $ref
top level?
I kind of like having at least the Operation
one as a definition as it will be the one we document outside the pgroll context.
I can get it done tomorrow so it's ready for a final review.
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.
After checking today, it might not be possible to generate these and re-use them.
- The
Operation
in pgroll it's not just an struct, it's an interface with methods. - The library hasn't added support for
anyOf
yet, they have a PR open but stale
I think it's fine as-is leaving the top level allOf: { $ref }
to reference the definition of PgRollMigration
that doesn't clash with the internal Migration
one.
I've subscribed to that PR, once they merge support for anyOf
maybe we can accomodate both and fully re-use Migration
, Operations
and Operation
.
WDYT?
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.
Thats fine from my POV. The main benefit we get from this is being able to generate the types for the different operations.
Is this PR good to go from your side now?
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.
Yes, can be merged when you're good with it
Do you have an idea in mind for how to generate documentation from the JSON schema? |
Co-authored-by: Andrew Farries <andyrb@gmail.com>
Signed-off-by: Alexis Rico <sferadev@gmail.com>
There are different tools to read JSON schemas and generate docs. Cloudflare has doca although doesn't seem maintained, Adobe has a markdown generator and there might be a few more. In any case, I was thinking of doing something a little bit more tailored for xata.io/docs. I think we will likely have 2 documentation sites: pgroll open source repo (and maybe even website in a future), and xata.io website for documenting the schema migrations in-product. It would be great if we make both share a single source of truth like the JSON Schema. |
All structs matched well except for
PrimaryKey
which we mapped differently, I've refactored it toPk
as it doesn't seem to support different names for go and JSON