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: Add support for message type generation from Json schemas #77

Conversation

jimmarino
Copy link
Contributor

What this PR changes/adds

This PR generates HTML tables containing relevant type information (e.g., required and optional properties) for all DSP messages. The HTML tables are generated by a custom Gradle Plugin that parses the DSP schemas at build-time. The generated tables may then be directly included in ReSpec markdown.

The PR introduces the plugin and Json Schema Object Model parser thatis used to generate the tables. Subsequent PRs will include the generated tables in the specification text and remove the relevant PNG images and manually-created type information.

Support for $comment or description still needs to be added. That can be done in another PR.

The plugin could be enhanced to provide hyperlinks for referenced type information. For example, a MessageOffer references the Offer type. The plugin could include a hyperlink directly to the Offer definition. We need to decide on the conventions for doing this.

Why it does that

Enhances spec consistency and reduces the chance for errors.

Further notes

List other areas of the documents that have changed but are not necessarily linked to the main feature. This could be editorial changes or mistakes in example files that were encountered and were fixed inline, etc.

Linked Issue(s)

Closes #62

@jimmarino
Copy link
Contributor Author

@arnoweiss

@mkollenstart
Copy link

The basis is I think great.

What kind of features do we want to ensure before merging this? Like you already mentioned are the links to the other schemas/tables. But apart from that some of the things I see when generating the tables:

  • profile is in the table any but in the schema it is a string or string array. (e.g. for Offer)
  • Enums are now just strings, but shouldn't the enum values themselves be mentioned in the table.
  • timestamp contains a format which is relevant. But might be difficult to present in a table, this can be mitigated by descriptions/comments.

I would suggest to make sure the tables are a bit more elaborate before including them in the spec, which can be done in separate PRs.

@jimmarino
Copy link
Contributor Author

The basis is I think great.

What kind of features do we want to ensure before merging this? Like you already mentioned are the links to the other schemas/tables. But apart from that some of the things I see when generating the tables:

  • profile is in the table any but in the schema it is a string or string array. (e.g. for Offer)
  • Enums are now just strings, but shouldn't the enum values themselves be mentioned in the table.
  • timestamp contains a format which is relevant. But might be difficult to present in a table, this can be mitigated by descriptions/comments.

I would suggest to make sure the tables are a bit more elaborate before including them in the spec, which can be done in separate PRs.

Yes I agree. @arnoweiss and I talked about this and came to the same conclusion. How about this approach:

  1. Merge the PR as a basis but do not include it in the specification text yet
  2. File issues (Arno and I have identified some, as well as the ones you outlined) and fix them
  3. Arno has a suggestion for improving the table format that he will file as a separate issue which we can implement
  4. Do a thorough review for correctness before including the generated tables in the specification text
  5. Include the tables and remove the PNGs

Let me know what you think.

@mkollenstart
Copy link

Sound like a good approach

@arnoweiss
Copy link
Contributor

Filed #80, #81 and #82

The generation process can be run by specifying the `generateTablesFromSchemas` task:

```
./gradlew generateTablesFromSchemas
Copy link
Contributor

Choose a reason for hiding this comment

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

This worked alright - additional feature requests are filed.

@jimmarino jimmarino merged commit 181ae8f into eclipse-dataspace-protocol-base:main Dec 4, 2024
2 checks passed
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.

succeed message-level puml images with generated property lists
3 participants