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

Fix go_option that contains both path and pkg #288

Merged
merged 2 commits into from
Jan 6, 2021

Conversation

marwan-at-work
Copy link
Contributor

@marwan-at-work marwan-at-work commented Nov 22, 2020

This PR adds a new check to the go_option parser to see if it includes a semi-colon. If it does, that means the user has already provided both the import path as well as the package name and that Twirp shouldn't try to deduce either of them.

In protobuf-go's v2, this is going to be the new default. See the referenced issue for more details.

Fixes #280

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@chetanvalagit
Copy link
Contributor

@marwan-at-work can you please add some detail description as part of PR?

Reference Example: #277

@marwan-at-work
Copy link
Contributor Author

@chetanvalagit done :)

@chetanvalagit
Copy link
Contributor

@marwan-at-work thank you very much. I am really sorry for the one more quick request: Can you please add last line too as shown in Reference Example: #277 if you are fine with that.

@chetanvalagit
Copy link
Contributor

@marwan-at-work thank you very much. I am really sorry for the one more quick request: Can you please add last line too as shown in Reference Example: #277 if you are fine with that

@marwan-at-work
Copy link
Contributor Author

@chetanvalagit done ✌️

@amcohen-twitch
Copy link
Contributor

Upon review, we're generally on board with this implementation. Could you please add a test to the existing tests for this function? https://github.com/twitchtv/twirp/blob/424ec823543050237314da4efe3ac642abb618f6/protoc-gen-twirp/go_naming_test.go

@marwan-at-work
Copy link
Contributor Author

@amcohen-twitch I don't think the link you sent me actually points to the function I modified. The link you sent is testing the parseGoPackageOption function which actually seems to be doing the correct logic. While, my function was missing the same logic.

Furthermore, that test function seems to be completely wrong, because it's shadowing all the test cases with empty strings.

Would you like me to create a new test for my own change, and also fix the tests you pointed out above?

@marwan-at-work
Copy link
Contributor Author

@amcohen-twitch I fixed the linked test cases and added new ones for my particular fix. Happy to change things up if I missed something here.

@advdv
Copy link

advdv commented Dec 29, 2020

Just wanted to say that i'm looking forward to this change. Hopefully it can be merged soon! Thank you

@marwan-at-work
Copy link
Contributor Author

@amcohen-twitch any luck on the above? Thanks!

@amcohen-twitch
Copy link
Contributor

@marwan-at-work Apologies for the delay in followup, we were out of office due to the holidays. Upon subsequent review, we're good with merging this.

@amcohen-twitch amcohen-twitch merged commit 0913c81 into twitchtv:master Jan 6, 2021
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.

detecting import paths is incorrect for root vanity imports
4 participants