-
Notifications
You must be signed in to change notification settings - Fork 135
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 OpenAPI3 strict references option to Committee (#343) #346
Conversation
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.
you may not be comfortable releasing this change on Committee mainline yet?
I think it's no problem :)
Do we need to provide a similar functionality for Hyper-schema?
We don't need to add it, some features are already available only in OpenAPI 3.
If anyone wants it, we should still work on another PR.
If this can fit into 5.x before it is released, then we can likely turn on true as a default from 6.x
🙆♀️ 🙆♀️ 🙆♀️ 🙆♀️
I think this change is OK!
But master branch is broken and I fixed it, so please rebase newest master 🙇
|
||
raise(ArgumentError, "Committee: need option `schema` or `schema_path`") unless schema | ||
if !schema && options[:schema_path] | ||
# In the future, we could have `parser_options` as an exposed config? |
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, I want to create config class like Committee::BaseOption, Committee::RequestOption, Committee::ResponseOption with types.
I think that these class can be used for type support, checking for correct values, and for internal use.
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.
If necessary, I would like to be able to have another smaller type as a value (BaseOption class have ParserOption class), or generate a class internally (BaseOption generate ParserOption and the user don't need to know ParserOption class).
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.
@ota42y Thanks for the feedback, I think I generally understand your direction. Is that something we should open a separate GitHub issue for and then think of an architecture there?
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, and I created new issue 🙋
#347
528edac
to
21f9634
Compare
@ota42y I changed the commit name as well as rebased off of master. |
Thanks!!!!! |
I released 5.0.0 with this changes! |
@ota42y please review my first implementation attempt to handle #343.
Issues I noticed while implementing:
This would force the
openapi_parser
Gem version to 1.0.0.beta, since that Gem is merely in beta, you may not be comfortable releasing this change on Committee mainline yet?Do we need to provide a similar functionality for Hyper-schema? I personally don't have any need for it in my work, but I can understand how it's nice if Committee can behave similarly for different schema types.
If this can fit into 5.x before it is released, then we can likely turn on
true
as a default from 6.x. I am wondering - is there any use case to even allow it to be configured as false? I guess after releasing this version, we may hear from users who want it to stay configurable.Also, similar to openapi_parser, I added
.ruby-version
and added.idea
to.gitignore
for RubyMine (JetBrains IDE).