-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix incorrect placement of header=present in versions endpoint #419
Conversation
Hmmm, I don't like that this changes the OpenAPI schemas, perhaps there is a way smarter way to pass these kind of parameters. |
Okay, replaced it with a slightly gross hack, think it should be stable enough. |
Codecov Report
@@ Coverage Diff @@
## master #419 +/- ##
=======================================
Coverage 90.99% 90.99%
=======================================
Files 55 55
Lines 2522 2522
=======================================
Hits 2295 2295
Misses 227 227
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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 would say yes, |
I agree with @sauliusg, the most correct thing to do is to include |
Just to clarify, we will definitely include it in the response (and even in the correct place this time), it's just whether it should be included in the OpenAPI schema, which would look like this: "content": {
"text/csv; header=present": {
"schema": {
"type": "string"
}
} This looks a bit funny to me, but I'm not sure of any other way of doing it. This also excludes the other parameter |
I think, let's add it to the OpenAPI as well. Being explicit. |
Fair enough, it certainly makes the fix easier! |
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.
Great catch with this one @ml-evs !
Closes #418