-
Notifications
You must be signed in to change notification settings - Fork 56
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 new Json schema properties #5
Add new Json schema properties #5
Conversation
feat: add "required" properties feat: add resolve metadata conf: add more rules in gitignore
Hello, If you want more details on the use of this new properties, you can check my module. Best regards, |
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.
Hi @FlorianDidier,
I like the idea of properly supporting the "required" and "default" attributes.
I'm not a fan of the generic "metaData" though since this kind of generic addition of attributes to a field or method is already supported via SchemaGeneratorConfigPart.withInstanceAttributeOverride()
.
E.g. in your module-addon
project, you'd only have to:
- replace the
.withMetadata(this::resolveMetadata)
call with.withInstanceAttributeOverride(this::resolveMetadata)
and - change the
resolveMetadata()
method to this:
protected void resolveMetadata(ObjectNode jsonSchemaAttributesNode, MemberScope<?, ?> member) {
this.getAnnotationFromFieldOrGetter(member, JsonSchema.class)
.map(JsonSchema::metadata)
.ifPresent(metaData -> Stream.of(metaData)
.filter(data -> !data.key().isEmpty())
.filter(data -> !data.value().isEmpty())
.forEach(data -> jsonSchemaAttributesNode.put(data.key(), data.value())));
}
That way you could theoretically also add the "required" and "default" attributes yourself already, but since those are standard attributes I like your proposed solution of supporting them specifically.
To summarise: after removing the changes related to the meta-data handling and cleaning-up some of the unnecessary style/import changes, I'm happy to merge this PR.
Thank you very much for your input. 👍
###################### | ||
# Gradle | ||
###################### | ||
.gradle/** |
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.
There is no Gradle in this project, but I like the idea of catering for more IDEs out-of-the-box.
import java.lang.reflect.Method; | ||
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
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.
We seem to have different configurations for the order of imports. 😉
I'd rather be consistent throughout the project and not include those order changes here.
* @param format attribute value to set | ||
* @return this instance (for chaining) | ||
*/ | ||
public AttributeCollector setDefault(ObjectNode node, String format) { |
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.
"format" as the variable name is a bit confusing to me here. it is the "defaultValue".
I like the general idea. I'll do the clean-ups myself and share the new PR (against the "master" branch) with you then. |
feat: add "default" value properties feat: add "required" properties feat: add resolve metadata conf: add more rules in gitignore
@FlorianDidier the support for the new "required" and "default" properties (together with the general support of the "pattern" property) have been released now as v3.2.0. As per my previous comment, I removed the proposed addition of a "metaData" map, as the existing Thank you for your contribution! |
feat: add "default" value properties
feat: add "required" properties
feat: add resolve metadata
conf: add more rules in gitignore