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

Mention Metadata.BinaryHeaderSuffix in xmldoc comments of Metadata.En… #2027

Merged
merged 3 commits into from
Feb 5, 2023
Merged

Mention Metadata.BinaryHeaderSuffix in xmldoc comments of Metadata.En… #2027

merged 3 commits into from
Feb 5, 2023

Conversation

voroninp
Copy link
Contributor

…try constructor and in message of ArgumentException. (#2025)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 26, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: voroninp / name: Pavel Voronin (fde7f2d)

@voroninp
Copy link
Contributor Author

Hm, locally tests passed, but here they are failing...

voroninp and others added 2 commits January 31, 2023 19:52
…tadata.Entry constructor and in message of ArgumentException. (#2025)
src/Grpc.Core.Api/Metadata.cs Outdated Show resolved Hide resolved
src/Grpc.Core.Api/Metadata.cs Outdated Show resolved Hide resolved
src/Grpc.Core.Api/Metadata.cs Outdated Show resolved Hide resolved
src/Grpc.Core.Api/Metadata.cs Outdated Show resolved Hide resolved
@JamesNK
Copy link
Member

JamesNK commented Feb 4, 2023

Hi, I like the improvement. A couple of changes:

  1. Just say what the -bin suffix is in comments.
  2. Replaced inhertdoc with copy and paste. These don't change much, and it's easier to see exactly what the comment when looking at source.

@voroninp
Copy link
Contributor Author

voroninp commented Feb 4, 2023

@JamesNK But if comments do not mention BinaryHeaderSuffix, won't people define the const one more time in their code?

@JamesNK
Copy link
Member

JamesNK commented Feb 5, 2023

@JamesNK But if comments do not mention BinaryHeaderSuffix, won't people define the const one more time in their code?

Good point. Updated comments. Thanks!

@JamesNK JamesNK merged commit fc4d087 into grpc:master Feb 5, 2023
@voroninp
Copy link
Contributor Author

voroninp commented Feb 6, 2023

@JamesNK Btw, it may be a bit extreme, but what do you think of renaming key to keyWithBinaryHeaderSuffix for the overload which accepts bytes?

public void Add(string keyWithBinaryHeaderSuffix, byte[] valueBytes)

@JamesNK
Copy link
Member

JamesNK commented Feb 6, 2023

I don't think it's necessary. Also, it's a breaking change for anyone using named parameters.

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

2 participants