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

Use constants for header names #1933

Merged
merged 2 commits into from
Sep 26, 2024
Merged

Use constants for header names #1933

merged 2 commits into from
Sep 26, 2024

Conversation

djc
Copy link
Contributor

@djc djc commented Sep 9, 2024

No description provided.

@tottoto
Copy link
Collaborator

tottoto commented Sep 19, 2024

Making the header name constant seems reasonable, but making them associated constants for Status doesn't seem like the good place to do so, considering that Status itself is not a header. Rather, given tonic::metadata::GRPC_CONTENT_TYPE is already placed as a header value constant, I feel it seems better to put it in tonic::metadata.

@LucioFranco
Copy link
Member

making them associated constants for Status doesn't seem like the good place to do so, considering that Status itself is not a header

I actually don't agree, we convert these headers in Status and its good to encapsulate the encoding portion of these header names within the status.rs. So to me this makes sense. Metadata is much more abstract than specific extraction of headers. In addition, the metadata shouldn't contain the status headers etc that should all be extracted from the Status functions.

tonic/src/status.rs Outdated Show resolved Hide resolved
@tottoto
Copy link
Collaborator

tottoto commented Sep 20, 2024

@LucioFranco

making them associated constants for Status doesn't seem like the good place to do so, considering that Status itself is not a header

I actually don't agree, we convert these headers in Status and its good to encapsulate the encoding portion of these header names within the status.rs. So to me this makes sense. Metadata is much more abstract than specific extraction of headers. In addition, the metadata shouldn't contain the status headers etc that should all be extracted from the Status functions.

I understand that the abstraction of metadata is a high level one. Then, I think it's fine to use Status's associated constants, considering that these will be doc(hidden) and will not actually be made into public API.

@djc
Copy link
Contributor Author

djc commented Sep 21, 2024

Made them #[doc(hidden)] as requested.

@LucioFranco LucioFranco added this pull request to the merge queue Sep 26, 2024
Merged via the queue into master with commit 517b7fc Sep 26, 2024
17 checks passed
@LucioFranco LucioFranco deleted the header-names branch September 26, 2024 16:00
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.

3 participants