Skip to content
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

[API Proposal]: HttpRequestOptions should implement IReadOnlyDictionary #68149

Closed
Grauenwolf opened this issue Apr 18, 2022 · 4 comments · Fixed by #86983
Closed

[API Proposal]: HttpRequestOptions should implement IReadOnlyDictionary #68149

Grauenwolf opened this issue Apr 18, 2022 · 4 comments · Fixed by #86983
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@Grauenwolf
Copy link

Grauenwolf commented Apr 18, 2022

Background and motivation

Methods and extension methods that don't modify a dictionary often expect a parameter of IReadOnlyDictionary instead of IDictionary.

API Proposal

namespace System.Net.Http
{
    public class HttpRequestOptions : IReadOnlyDictionary<string,object>, // NEW
                                      IDictionary<string, object> // EXISTING
    {
          [...]
    }
}

API Usage

var dic = (IReadOnlyDictionary<string,object>)request.Options;

Alternative Designs

No response

Risks

In rare circumstances, a method will have an overload for IDictionary and another overload for IReadOnlyDictionary. This would be ambiguous for HttpRequestOptions, as it is for other dictionaries.

@Grauenwolf Grauenwolf added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Apr 18, 2022
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Apr 18, 2022
@ghost
Copy link

ghost commented Apr 18, 2022

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

Issue Details

Background and motivation

Methods and extension methods that don't modify a dictionary often expect a parameter of IReadOnlyDictionary instead of IDictionary.

API Proposal

namespace System.Net.Http
{
    public class HttpRequestOptions  : IReadOnlyDictionary<string,object>
    {
          [...]
    }
}

API Usage

var dic = (IReadOnlyDictionary<string,object>)request.Options;

Alternative Designs

No response

Risks

In rare circumstances, a method will have an overload for IDictionary and another overload for IReadOnlyDictionary. This would be ambiguous for HttpRequestOptions, as it is for other dictionaries.

Author: Grauenwolf
Assignees: -
Labels:

api-suggestion, area-System.Collections, untriaged

Milestone: -

@ghost
Copy link

ghost commented Apr 18, 2022

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

Issue Details

Background and motivation

Methods and extension methods that don't modify a dictionary often expect a parameter of IReadOnlyDictionary instead of IDictionary.

API Proposal

namespace System.Net.Http
{
    public class HttpRequestOptions  : IReadOnlyDictionary<string,object>
    {
          [...]
    }
}

API Usage

var dic = (IReadOnlyDictionary<string,object>)request.Options;

Alternative Designs

No response

Risks

In rare circumstances, a method will have an overload for IDictionary and another overload for IReadOnlyDictionary. This would be ambiguous for HttpRequestOptions, as it is for other dictionaries.

Author: Grauenwolf
Assignees: -
Labels:

api-suggestion, area-System.Net.Http, untriaged

Milestone: -

@karelz
Copy link
Member

karelz commented Apr 21, 2022

Triage: IReadOnlyDictionary is strict subset of already implemented IDictionary, so the work is minimal. We wouldn't mind having both interfaces, unless it brings some known problems in general (API review to advise here).
BTW: The type HttpRequestOptions was introduced in .NET 5.

@karelz karelz modified the milestones: 7.0.0, Future Apr 21, 2022
@karelz karelz added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Apr 21, 2022
@bartonjs
Copy link
Member

Looks good as proposed.

We discussed the risks inherent to being both IDictionary and IReadOnlyDictionary (namely, being an ambiguous target for overloading). But we generally think that the risk is low, and the workaround (casting to one of the two interfaces) is easy.

namespace System.Net.Http
{
    public class HttpRequestOptions :
+        IReadOnlyDictionary<string,object>,
         IDictionary<string, object>
    {
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels May 11, 2023
@MihaZupan MihaZupan added the help wanted [up-for-grabs] Good issue for external contributors label May 11, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 31, 2023
xrem added a commit to xrem/runtime that referenced this issue Jun 1, 2023
rzikm pushed a commit that referenced this issue Jun 21, 2023
* Added IReadOnlyDictionary to HttpRequestOptions

Closes #68149

* Added test for new methods

* Added negative scenario to HttpRequestOptionsTest

Co-authored-by: Stephen Toub <stoub@microsoft.com>

---------

Co-authored-by: Stephen Toub <stoub@microsoft.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 21, 2023
@karelz karelz modified the milestones: Future, 8.0.0 Jul 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Net.Http help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants