Skip to content

Added IReadOnlyDictionary to HttpRequestOptions - #86983

Merged
rzikm merged 3 commits into
dotnet:mainfrom
xrem:xrem/http-request-options-readonly-dictionary
Jun 21, 2023
Merged

Added IReadOnlyDictionary to HttpRequestOptions#86983
rzikm merged 3 commits into
dotnet:mainfrom
xrem:xrem/http-request-options-readonly-dictionary

Conversation

@xrem

@xremxrem commented May 31, 2023

Copy link
Copy Markdown
Contributor

Closes#68149

@ghostghost added area-System.Net.Http community-contribution Indicates that the PR has been added by a community member labels May 31, 2023
@ghost

ghost commented May 31, 2023

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Closes #68149

Author:xrem
Assignees:-
Labels:

area-System.Net.Http

Milestone:-

@xrem

xrem commented May 31, 2023

Copy link
Copy Markdown
ContributorAuthor

@dotnet-policy-service agree

@xrem

xrem commented Jun 1, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

How do I generate CombatibilitySupression.xml for System.Net.Http? 🤔

Running dotnet build ApiCompat.proj /p:ApiCompatGenerateSuppressionFile=true inside src\libraries\apicompat as task output says, does not produce supression for related changes.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=291697&view=logs&j=4590e2f8-699e-5f49-61ce-34aa015d5222&t=9a53ce37-8f72-55d0-fdf3-f3f4cb573bd0&l=2147

stephentoub
stephentoub reviewed Jun 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpRequestOptions.cs Outdated
stephentoub
stephentoub reviewed Jun 1, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/src/System/Net/Http/HttpRequestOptions.cs Outdated
This was referenced Jun 1, 2023
Closed
Closed
@xrem
xremforce-pushed the xrem/http-request-options-readonly-dictionary branch from 3c67669 to f9b3953CompareJune 1, 2023 18:53
@xrem
xrem marked this pull request as ready for review June 1, 2023 20:59
@xrem
xrem requested a review from stephentoubJune 1, 2023 20:59
@xrem

xrem commented Jun 1, 2023

Copy link
Copy Markdown
ContributorAuthor

Looks like failed checks should pass, but I don't know how to dispatch them to run again :)

This was referenced Jun 1, 2023
Closed
Closed
stephentoub
stephentoub reviewed Jun 2, 2023
View reviewed changes

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Can you add tests, too?

@xrem

xrem commented Jun 2, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

My tests passed, but there is still some issues with infrastructure like this:
image
Log

So, it's ready to review.

@xrem
xrem requested a review from stephentoubJune 2, 2023 18:31
stephentoub
stephentoub reviewed Jun 5, 2023
View reviewed changes
Comment threadsrc/libraries/System.Net.Http/tests/UnitTests/Headers/HttpRequestOptionsTest.cs Outdated
@xrem@stephentoub
Added negative scenario to HttpRequestOptionsTest
0ec6983
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@xrem
xrem requested a review from stephentoubJune 6, 2023 06:30
stephentoub
stephentoub approved these changes Jun 8, 2023
View reviewed changes

@stephentoubstephentoub left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks.

@stephentoub
stephentoub requested a review from a teamJune 8, 2023 15:40
@rzikm

rzikm commented Jun 20, 2023

Copy link
Copy Markdown
Member

/azp run runtime

@azure-pipelines

azure-pipelinesBot commented Jun 20, 2023

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

rzikm
rzikm approved these changes Jun 20, 2023
View reviewed changes

@rzikmrzikm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, lets wait for the CI and then we're good to merge

@rzikm

rzikm commented Jun 21, 2023•
edited
Loading

Copy link
Copy Markdown
Member

CI Failure is unrelated. We're good to go. Thank you for the contribution!

@rzikm
rzikm merged commit 19a088e into dotnet:mainJun 21, 2023
@xrem
xrem deleted the xrem/http-request-options-readonly-dictionary branch June 22, 2023 08:24
@karelzkarelz added this to the 8.0.0 milestone Jul 3, 2023
@ghostghost locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@stephentoubstephentoubstephentoub approved these changes
@rzikmrzikmrzikm approved these changes

Assignees

No one assigned

Labels

area-System.Net.Httpcommunity-contributionIndicates that the PR has been added by a community member

Projects

None yet

Milestone

8.0.0

Development

Successfully merging this pull request may close these issues.

[API Proposal]: HttpRequestOptions should implement IReadOnlyDictionary

4 participants

@xrem@rzikm@stephentoub@karelz