Refactoring ProtobufGen to use SimpleCodeGen. #1613
Conversation
| @@ -103,55 +86,44 @@ def resolve_deps(self, deps_list, key): | |||
| .format(message=e, key=key)) | |||
| return deps | |||
|
|
|||
| def invalidate_for_files(self): | |||
| return [self.protobuf_binary] | |||
|
|
|||
| @property | |||
| def javadeps(self): | |||
| return self.resolve_deps(self.get_options().javadeps, 'javadeps') | |||
There was a problem hiding this comment.
nit: I think it might have been important to make this a property when javadeps was stored as a local attribute and the resolve logic was more extensive
self._javadeps = ...
but now the code is so concise, maybe we should just inline it into synthetic_target_extra_dependencies (unless there is a reason you want to unit test it)
There was a problem hiding this comment.
I just left it in because test_protobuf_gen already had working unit tests for it, and I didn't see a reason to break that.
|
The only major thing is the issue in simple_codegen_task.py, go ahead and put up the public review! |
| self.context.log.debug('Jar already extracted at {jar_path}.'.format(jar_path=jar_path)) | ||
| return outdir | ||
| def execute_codegen(self, targets): | ||
| if not targets: |
There was a problem hiding this comment.
Is this an important change? For some reason I thought that sometimes this task might get called with no targets. In any case, do we really want to error out? Maybe just print a log message and return. It isn't anything wrong with this task (maybe the execution engine.)
gmalmquist
force-pushed
the
gmalmquist/protobuf-simple-codegen
branch
3 times, most recently
from
277593f
to
8a4fa29
Compare
|
Changes Unknown when pulling 8a4fa29 on gmalmquist:gmalmquist/protobuf-simple-codegen into * on pantsbuild:master*. |
gmalmquist
force-pushed
the
gmalmquist/protobuf-simple-codegen
branch
from
8a4fa29
to
23ae942
Compare
At jsirois's suggestion, the python code-gen sections of ProtobufGen were dropped due to disuse and poor semantics (since it was generating python code from java_protobuf_libraries). Test-cases were modified to accomodate modified output due to generating fewer languages. I had to modify jar_publish slightly to pass an integration test in jar_publish_integration that was publishing jars created from protoc libraries.
gmalmquist
force-pushed
the
gmalmquist/protobuf-simple-codegen
branch
from
23ae942
to
452358a
Compare
1 similar comment
At jsirois's suggestion, the python code-gen sections of ProtobufGen
were dropped due to disuse and poor semantics (since it was
generating python code from java_protobuf_libraries).
Test-cases were modified to accomodate modified output due to
generating fewer languages.
I had to modify jar_publish slightly to pass an integration test in
jar_publish_integration that was publishing jars created from protoc
libraries.