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 withObjectMapper method to customize serialization of schemas to file #388

Merged
merged 3 commits into from
Oct 25, 2023

Conversation

takanuva15
Copy link
Contributor

Hi, I put together this rough PR based on our discussion in #384.

3 quick questions on this:

  1. Assuming the logic looks fine to you, can you clarify which test classes I need to update? (So far I found SchemaGeneratorConfigBuilderTest.java).
  2. Do I need to update the github.io documentation you have published on the gh-pages branch?
  3. Is it worthwhile to add an example within jsonschema-examples demonstrating the customization of the ObjectMapper to print yaml?

Copy link
Member

@CarstenWickner CarstenWickner left a comment

Choose a reason for hiding this comment

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

Thanks for raising this PR 🙇

To answer your questions:

  1. The general logic fine, yes. I just added two minor comments. I have no particular test classes in mind, but yes the SchemaGeneratorConfigBuilderTest would be good for completeness' sake. The change is not that big I suppose.
  2. I can take care of the documentation afterward. For reference though, it's defined as markdown files in the slate-docs/source/includes directory of this repository.
  3. Examples for the Maven plugin are somewhat tricky in the jsonschema-examples scope. An example of using YAML as the output format would be good to include in the FAQs of the documentation I suppose. Feel free to add it yourself, otherwise, I'll do it later.

@takanuva15
Copy link
Contributor Author

To answer your questions:
...

Thanks for the reply. I've updated the PR and left the documentation untouched.

This change is really only relevant to the maven plugin, because if a dev is manually making their own schema through the builder, then they'll have access to the root JsonNode and can serialize it however they want.

In that sense, perhaps the documentation would make more sense to go under _maven-plugin.md. In any case, I'll leave it to you to configure it as you like.

Copy link
Member

@CarstenWickner CarstenWickner left a comment

Choose a reason for hiding this comment

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

Looking good now.
Adding this to the documentation of the Maven plugin is a fair point.
Users are more likely to find it there than in the FAQs.

I'll do that in a separate PR then.
Thank you for this contribution. 😄👍

@CarstenWickner CarstenWickner marked this pull request as ready for review October 25, 2023 20:02
@CarstenWickner CarstenWickner merged commit 42cea4d into victools:main Oct 25, 2023
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants