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

Add IReadOnlyDictionary to BalancerAttributes collection #2121

Merged

Conversation

JamesNK
Copy link
Member

@JamesNK JamesNK commented May 11, 2023

Make this collection feel a little more idiomatic to use. Some extension methods in .NET hang off the readonly interface and assume all dictionaries implements IDictionary and IReadOnlyDictionary.

@@ -29,7 +30,7 @@ namespace Grpc.Net.Client.Balancer;
/// Note: Experimental API that can change or be removed without any prior notice.
/// </para>
/// </summary>
public sealed class BalancerAttributes : IDictionary<string, object?>
public sealed class BalancerAttributes : IDictionary<string, object?>, IReadOnlyDictionary<string, object?>

Choose a reason for hiding this comment

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

Assuming we don't need an API review for adding this interface.

Copy link

@mitchdenny mitchdenny left a comment

Choose a reason for hiding this comment

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

LGTM but I don't know whether this qualifies for needing an API review or not. But seems reasonable.

@JamesNK
Copy link
Member Author

JamesNK commented May 12, 2023

This type is basically a copy of HttpRequestOptions. And they've already done the API review 😄

dotnet/runtime#68149

@JamesNK JamesNK merged commit 2dc971e into grpc:master May 16, 2023
5 checks passed
@JamesNK JamesNK deleted the jamesnk/attributescollection-readonlydictionary branch May 16, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants