-
Notifications
You must be signed in to change notification settings - Fork 94
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: add response headers to HttpException #658
Conversation
/// <summary> | ||
/// The response headers | ||
/// </summary> | ||
public IEnumerable<HeaderParameter> Headers; |
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 do not have a strong opinion on this, but since Headers
property in RestResponse
class is nullable (T?
), I think it should be here too, ie.
public IEnumerable<HeaderParameter> Headers; | |
public IEnumerable<HeaderParameter>? Headers { get; set; } |
And, using a property is more common than a plain field, and follows the style of existing code.
Otherwise good to go after CHANGELOG is updated. The amount of test coverage of the new feature is outstanding.
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.
Added as suggested
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.
LGTM now!
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.
LGTM 🚀
Proposed Changes
HttpException
WriteErrorEvent
Checklist
dotnet test
completes successfully