-
Notifications
You must be signed in to change notification settings - Fork 624
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
Update dependencies to support Generics #360
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). For more information, open the CLA check for this pull request. |
signed ⚡ ✍️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Enrique!
Currently it requires getMessageResolver
in wire.go
to make wire happy. Ideally someone would want to say wire.Build(NewEvent, NewGreeter, getMessageResolver[bool, Message])
and expect things to just work. But this change is definitely a solid step forward, so LGTM!
Great @jayzhuang . should we merge this PR or wait until the whole functionality is there? (I'm now working with my forked repo and would love to come back to using the original |
Hey Enrique, I'm supportive of merging this PR, but I don't have write access. @stytchiz can you take a look? |
One vote for this PR, 👍 . |
Understood, thanks for the support @jayzhuang |
From the description ...
Can you provide more details on what use case this PR is solving (as well as not solving)? I can take a deeper look during the weekend. My major concern is that this PR will down the line require supporting generics as a new feature which we don't have the bandwidth for at the moment. |
Thanks for your comment @stytchiz. I understand the concern about bandwidth and it totally makes sense. Regarding
This was mostly a disclaimer or cautionary comment, since I didnt try all possible options involving generics. In the first comment I provided an example where To my understanding, this change doesnt include complete support for generics (like @jayzhuang commented): you cannot do The code from #354 still doesnt work with this change, however they would have a workaround. They just have to remove the generics from Example from #354 that works changing their wire.go to this:
Previous wire.go
Let me know if you need anything else and thanks for your support |
Hello @stytchiz, I understand that you are very busy but I wanted to check if you had a chance to review the comment or if you need further information. |
Sorry for the delay @efueyo ! Your response was very helpful. LGTM. The changes adds a new switch case for I'll also update #354 that this is a workaround and that full support for generics is not on the table. |
Thanks for your support and c
No need to apologize, I understand we all might be very busy. Thanks for finding the time to carefully look at this, though! |
Hello! First of all, thanks for this project. It is very helpful and makes dependency injection a breeze.
We've experience some problems working with wire and generics (as I think other people has experienced too according to #354)
I dont know for sure if these changes added full support for generics but I've managed to fix all the issues I had in my projects.
Mainly, I had to update the dependencies to use
golang.org/x/tools@v0.1.10
The main issue was here:
https://github.com/golang/tools/blob/master/go/ast/astutil/rewrite.go#L255
that was included in this commit
this is a version adapted from the tutorial that works only with the updated version: