-
Notifications
You must be signed in to change notification settings - Fork 195
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
HTTP Request and Response updates #150
Conversation
9390eca
to
457b17e
Compare
_cookies = cookies; | ||
} | ||
|
||
public override void Append(string name, string value) |
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.
Should value be an optional string? value
? Per here: https://docs.microsoft.com/en-us/dotnet/api/system.net.cookie.-ctor?view=net-5.0#System_Net_Cookie__ctor_System_String_System_String_
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 a bit odd... if you look at the remarks for that documentation, it states that the value cannot be null
(should be an empty string if not set). The exceptions documentation there also states that a null ref exception will be thrown if the value is null 🤷🏼♂️
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.
@fabiocav - I think from the code that null will become empty string by default: https://github.com/dotnet/runtime/blob/4893645536af64b01884ce5f3c392ad63513d04b/src/libraries/System.Net.Primitives/src/System/Net/Cookie.cs#L669
Found a related PR here: dotnet/corefx#8206 (which resolves this issue)
And an issue on the docs saying it will throw for null, which appears to be resolved but the docs look the same? :|
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.
Yeah, looks like a doc issue.
|
||
public bool? Secure { get; set; } | ||
|
||
public string Value { get; set; } |
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.
Same as earlier comment: string? Value
?
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.
Same comment as above..... not sure we should add...
457b17e
to
c69d63c
Compare
No description provided.