plugin_modules: add plugin_module_deps for supporting java_library deps#445
Conversation
robfig
requested review from
alexeagle,
gregmagolan and
soldair
as code owners
Yannic
left a comment
Yannic
left a comment
There was a problem hiding this comment.
Thanks! Added some comments.
|
Thanks for the review. Still working on update to include |
robfig
force-pushed
the
plugin_modules_pr
branch
2 times, most recently
from
df6fc52 to
822ccbf
Compare
robfig
left a comment
robfig
left a comment
There was a problem hiding this comment.
@Yannic Updated to use a plugin structure. Despite it being super simple, I had a lot of doubts on the details where I felt like I was making arbitrary choices. I left comments about them, so feel free to weigh in, or if you are also indifferent / think it doesn't matter, then that's good to know too. Thanks and happy 🦃
🦃 🦃 🦃
Yannic
left a comment
Yannic
left a comment
There was a problem hiding this comment.
This looks pretty good! Just a few more comments.
robfig
force-pushed
the
plugin_modules_pr
branch
from
6eba2e2 to
e3f8543
Compare
|
Thanks for the review - addressed all your comments. PTAL |
Yannic
left a comment
Yannic
left a comment
There was a problem hiding this comment.
It seems like closure_templates_plugin.bzl was not pushed.
Also, please remember to export closure_templates_plugin in //closure:defs.bzl.
robfig
force-pushed
the
plugin_modules_pr
branch
2 times, most recently
from
203bfe2 to
1ba2613
Compare
|
Thanks - updated and squashed |
robfig
force-pushed
the
plugin_modules_pr
branch
from
1ba2613 to
9fe6e68
Compare
|
Removed the now-failing test for the print directive, indicated by the build failure |
Yannic
left a comment
Yannic
left a comment
There was a problem hiding this comment.
I'm feeling very very bad for repeatedly asking you for changes, but there's one more thing :/.
robfig
force-pushed
the
plugin_modules_pr
branch
from
9fe6e68 to
42419a1
Compare
robfig
left a comment
robfig
left a comment
There was a problem hiding this comment.
Nonsense, this is how learning and development works. I appreciate your comments, thank you.
Yannic
left a comment
Yannic
left a comment
There was a problem hiding this comment.
I absolutely agree, but some people get frustrated if they have to go through many rounds of review and I wanted to make sure you don't feel that way. Glad you're happy about it and thank you for the contribution.
This LGTM now, deferring to @alexeagle for authoritative review and merging.
robfig
force-pushed
the
plugin_modules_pr
branch
from
42419a1 to
0a82c56
Compare
|
Thank you! |
alexeagle
force-pushed
the
plugin_modules_pr
branch
from
0a82c56 to
c7247a3
Compare
|
seems fine to me. internally we call this attribute "plugins". I am guessing this doesn't work for javascript right? |
|
@iteriani Right now, this only works for Javascript. To make it work for Java, we'd also need to modify Renaming to plugins sgtm. |
robfig
force-pushed
the
plugin_modules_pr
branch
from
c7247a3 to
a5ce474
Compare
|
Renamed to plugins |
alexeagle
force-pushed
the
plugin_modules_pr
branch
from
a5ce474 to
5523696
Compare
Pass classpath and option to Soy compiler in closure_js_template_library. Add unit test to confirm it works with a custom function. Rename plugin_modules to plugins to match Google's internal rule. Fixes bazelbuild#197
alexeagle
force-pushed
the
plugin_modules_pr
branch
from
5523696 to
b02a9e0
Compare
|
Found an issue, here's a fix: |
5 participants
Pass classpath and option to Soy compiler.
Add unit test to confirm it works with a custom print directive and function.
Fixes #197