-
Notifications
You must be signed in to change notification settings - Fork 83
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
PathItem and PathItemOperation annotations #629
Conversation
/** | ||
* The tag names which apply to this operation. | ||
* | ||
* @return the list of tag names | ||
*/ | ||
String[] tags() default {}; // Could be @Tag[] | ||
|
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.
I remembered there was one bit I wasn't sure about and it was this.
At the moment, we allow @Tag
to be put on a rest resource method and it will tag the method and create a tag entry at the top level. The user can put a description for the tag in there as well, but if they put a different description in different places then it's undefined which one ends up in the final document.
We could replicate that function here, or we could require the user to just use the tag name, and use the @Tag
annotation separately if they want a description.
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.
Decided to use the @Tag
annotation here. It's a bit more verbose but it's consistent with how tags are added to operations elsewhere.
I've done it as a separate commit so it can be easily removed if needed.
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.
+1 to how you've done it.
16e49bc
to
65919a6
Compare
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.
I think this looks good, thanks. I just have one comment that might require an adjustment and one with me thinking out loud about the Callback
model 😄
* This property provides a reference to an object defined elsewhere. This property and properties other than | ||
* {@link #name()} are mutually exclusive. If properties other than {@code name} are defined in addition to the | ||
* {@code ref} property then the result is undefined. |
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.
Mutual exclusivity between $ref
and other properties isn't the case for path items, right?
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.
Ah, good spot. Will need a test for this too.
* | ||
* @since 4.0 | ||
*/ | ||
String pathItemRef() default ""; |
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.
It's hard to get my head around a Callback
being a reference to either another Callback
or a PathItem
. Is it correct to say that our Callback
is effectively a map of PathItem
s? So, it's basically just one level above a PathItem
, which is why using the pathItemRef
requires the callbackUrlExpression
to complete the "entry".
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.
Yep, that's pretty much it.
We could probably do with some examples in the class Javadoc, though we could do with examples on every annotation, along with saying where to use it and more precisely what it does.
Allows PathItems to be defined under Components. Allows webhooks to be defined.
- Define a PathItem under Components - test all parameters - Define Operations under a Path Item - test all parameters - Define a webhook - Reference a Path Item from a webhook - Reference a Path Item from a callback - Reference a Path Item from another Path Item under Components
fc47197
to
9117ff2
Compare
Requested changes made and commits squashed. |
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.
Looks good, thanks.
Fixes #588
Fixes #583
Fixes #437