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

reflection: Fix references to symbols with no package #2678

Merged
merged 1 commit into from Feb 27, 2024

Conversation

murgatroid99
Copy link
Member

This is a follow up to #2673, and fixes #2671 according to my manual testing. The new comment basically explains this change:

In files with no package, symbols are populated in the symbols table with a leading period in the key. If there is a package, the symbol does not have a leading period in the key. For simplicity, we check without the period, then with it.

CC @jtimmons

@murgatroid99 murgatroid99 merged commit cfa8072 into grpc:master Feb 27, 2024
9 of 10 checks passed
@jtimmons
Copy link
Contributor

ah good catch @murgatroid99 - the "absolute vs relative" package references are definitely a bit confusing because we need to handle it on both ends (how it's inserted into the index and then how it's accessed afterwards) and it doesn't seem super consistent across those as to which reference type is used. I think #2595 would solve this as we'd be working off the source-of-truth at that point rather than trying to determine the file relationships after load-time

another thing we could do here is add an option to the service to suppress these warnings? They don't always have a material effect on a reflection client working so in some cases it might just be noise for the user

@murgatroid99
Copy link
Member Author

another thing we could do here is add an option to the service to suppress these warnings? They don't always have a material effect on a reflection client working so in some cases it might just be noise for the user

I was actually thinking that maybe they should be errors instead of warnings. These warnings indicates that API responses will have incorrect dependency information, and it probably indicates a programming error, either on our part or on the part of the reflection library user.

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.

@grpc/reflection writes wanings to console for .proto files with no package declaration
3 participants