-
Notifications
You must be signed in to change notification settings - Fork 251
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
Support xml error respsonse #1631
Conversation
b844756
to
8c83a17
Compare
5b55f97
to
ad3f688
Compare
593db29
to
02b0357
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.
Looking much better, but a few changes would be good.
Co-authored-by: Heath Stewart <heaths@outlook.com>
Co-authored-by: Heath Stewart <heaths@outlook.com>
Co-authored-by: Heath Stewart <heaths@outlook.com>
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.
Please revert the headers
change and it looks good to merge. We want to keep it as that type because callers can always convert, but Headers
may provide additional APIs a simple HashMap
may not.
This reverts commit a2f3b4b.
closes #1275
As mentioned in the issue for those places that only need the error code it would've been easier to get it from the headers but I'm unsure whether there are any case where this information is only provided through the body.
So to keep the current behavior I decided to keep parsing it from the body (now based on
content-type
though)If you think it would be alright to rely solely on the headers than I'd be happy to change that, it would remove a bit of code and prevent having to parse the body in some cases.
EDIT: Thanks to the all_tests script I discovered the issue of
quick-xml
being behind thexml
feature flag.I see two options but don't know which one is better:
quick-xml
/ remove thexml
featurexml
feature (needs documentation)To quick fix for CI I will remove
quick-xml
from thexml
feature for now. I'm thankful for any further guidance here