Skip to content

Extract out parsing proto file logic from ProtobufGen - #755

Closed
alyssa wants to merge 3 commits into
pantsbuild:masterfrom
alyssa:alyssa/refactor+protobuf+gen
Closed

Extract out parsing proto file logic from ProtobufGen#755
alyssa wants to merge 3 commits into
pantsbuild:masterfrom
alyssa:alyssa/refactor+protobuf+gen

Conversation

@alyssa

@alyssa alyssa commented Oct 31, 2014

Copy link
Copy Markdown

No description provided.

@alyssa alyssa changed the title Extract out parsing proto file logi[Cc from ProtobufGen Extract out parsing proto file logic from ProtobufGen Oct 31, 2014
@alyssa
alyssa force-pushed the alyssa/refactor+protobuf+gen branch from 0bcf53c to 0408d4f Compare October 31, 2014 14:48

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we also pass the service list into this method?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't have to. The protobuf compiler always generates java classe for services. The wire compiler optionally does this, thus we'll have to pass in service_writer when calling this in wire_gen

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.05%) when pulling 0408d4f on alyssa:alyssa/refactor+protobuf+gen into aa631e3 on pantsbuild:master.

@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.05%) when pulling 0408d4f on alyssa:alyssa/refactor+protobuf+gen into aa631e3 on pantsbuild:master.

@alyssa
alyssa force-pushed the alyssa/refactor+protobuf+gen branch from 0408d4f to 1965550 Compare October 31, 2014 18:01

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The name 'type' shadows a builtin
  2. Let's not use '%' style formatting, its going away in Python 3
  3. Now that I see it this way, there is a shorthand that might make it look better.

classnames = set(['{name}OrBuilder'.format(name=t) for t in protobuf_parse.messages])
classnames = classnames.union(protobuf_parse.messages)

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay for list comprehension, made the change

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.

3 participants