Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Added support for weak imports in ProtoParser, ProtoFile and ProtoFileElement #3227

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

paul35621
Copy link

@paul35621 paul35621 commented Dec 31, 2024

I changed ProtoParser, ProtoFile and ProtoFileElement to support weak imports.

I didn't try to compile this, but if it does compile then the new behaviour would be that weak imports are ignored, since I added a field weakImports to ProtoFile and ProtoFileElement that is not read outside of these classes. The current situation is that parsing fails when the file contains a weak import, so I consider that an improvement.

See issue 3226.

@JakeWharton
Copy link
Collaborator

Needs a test, but otherwise looks fine.

@paul35621 paul35621 changed the title Added support for weak imports in ProtoParser and ProtoFileElement Added support for weak imports in ProtoParser, ProtoFile and ProtoFileElement Dec 31, 2024
@oldergod
Copy link
Member

oldergod commented Jan 6, 2025

I can't push to this branch but I've modified an existing test here: https://github.com/square/wire/compare/bquenaudon.2025-01-06.weakimports?expand=1
also dumped API and spotless

@paul35621
Copy link
Author

Alright, great! Should I integrate that in this pull request, or what would be the next step?

@JakeWharton
Copy link
Collaborator

Yes, please add a test to this PR so we can validate that it and the others pass with the new changes.

@paul35621
Copy link
Author

@JakeWharton Can you edit this pull request or make a new one? The code with the test is already on the bquenaudon.2025-01-06.weakimports branch. I've tried to figure out how to make a pull request, but it's above my head.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants