-
-
Notifications
You must be signed in to change notification settings - Fork 186
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: adding interpreter options override #1071
Conversation
@jonaslagoni this one is back 😄 . Could you please take a look. Thank you. |
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.
@artur-ciocanu nice!
We do need some extra changes though, cause without them, the users won't the feature is there! So we need to make the documentation reflect the use-case and then provide an example where you show how it can be done 🙂
This is really super useful. |
@jonaslagoni I am still new to this, so could you please let me know where should I add the examples and the docs? |
@artur-ciocanu you can find out how to create new examples here: https://github.com/asyncapi/modelina/blob/next/docs/contributing.md#adding-examples For the docs it's slightly trickier, I think we need a section here: https://github.com/asyncapi/modelina/blob/next/docs/advanced.md It should explain the use-case of when you would want to use this feature of overwriting the interpreters as well as a link to the example you created 🙂 |
5ef8efc
to
095dff7
Compare
@jonaslagoni I have added an example and a new section to docs. Let me know if it is looking good. Also should I rebase from |
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.
Something I have been wondering, cant recall if we already discussed this, but if it's really only about turning interpretation on or off for additionalProperties
and additionalItems
, we could just add the options ignoreAdditionalProperties
and ignoreAdditionalItems
instead of allowing changing the entire interpretation callbacks.
It would be both easier for the end-user and easier to implement.
But if the use-case is really to allow custom interpretations, that can't be solved by simple boolean options, then I am all aboard this feature. Just need to be for the right reasons 😄
I actually cant recall what needs to change once we merge #674, so lets see 🙂 |
@jonaslagoni now that you mentioned it, I do think that in some cases we might want to have the full power of interpreter options overwrite. However I would go with simple booleans for I will adjust the PR, so it is smaller in scope and I'll adjust the examples and docs. Thanks a lot for a great suggestion! |
Sounds good @artur-ciocanu 👍 Also feel free to re-target the PR to the |
…ll options override
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Closing this one in favor of #1094. @jonaslagoni unfortunately there were too many conflicts. |
Description
This is an enhancement that would allow anyone to override default interpreter behavior. Some of use cases are overriding default behavior of
additionalProperties
,additionalItems
that would allow anyone to customize how these schema properties are handled. For example by default these properties are treated astrue
, however we might want to treat them asfalse
during code gen.A typical example using interpreter options would look like this:
Related issue(s)
#916