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

desc/protoparse: report unused imports #403

Merged
merged 1 commit into from Jun 16, 2021
Merged

Conversation

jhump
Copy link
Owner

@jhump jhump commented Jun 16, 2021

This is a backport of #355 to master. That other pull request is based on a big refactor, for better interop with the newest protobuf runtime reflection stuff. There's enough risk (mostly in performance regressions) in that refactor (which is in #354), that I backported this change so we can land it without having to first land the refactor...

This also includes a change to support using a custom version of descriptor.proto, which is something else that protoc supports and that was also added in that refactor PR.

Resolves #281.
And also resolves #360.

@jhump
Copy link
Owner Author

jhump commented Jun 16, 2021

cc @bufdev

return md
}

// TODO: should this support public imports and be recursive?
Copy link
Contributor

Choose a reason for hiding this comment

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

just making sure nothing to do here

Copy link
Owner Author

Choose a reason for hiding this comment

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

I think this will involve a little more toying around with protoc to understand any nuances of its behavior and then mimic it exactly.

Also, FWIW, this is only for addressing #360 (using custom descriptor.proto), not for implementing the warning on unused imports. I addressed that issue at the same time just since my previous PR happened to already have test cases that cover the custom descriptor.proto case. This is a very rarely used feature of protoc (only useful if you are working on protoc or forking it and making changes to descriptor.proto). So this PR will at least get protoparse closer even if not yet perfect; so I don't think we need to do anything here right now.

@bufdev
Copy link
Contributor

bufdev commented Jun 16, 2021

LGTM

@jhump jhump merged commit 6cc1efa into master Jun 16, 2021
5 checks passed
@jhump jhump deleted the jh/report-unused-imports branch June 16, 2021 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants