Skip to content
This repository has been archived by the owner on May 4, 2019. It is now read-only.

Protobuf 3.2.0 #82

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Protobuf 3.2.0 #82

wants to merge 4 commits into from

Conversation

ljdursi
Copy link

@ljdursi ljdursi commented Mar 29, 2017

Bumps protobuf version to 3.2.0, which means C++ implementation on Linux and so faster/reduced memory usage. Addresses issue ga4gh/ga4gh-server#1623.

The C++ implementation is fussier than the python version - this means had to explicitly create a "proper" FakeObject for tests/unit/test_cli.py. It also means that pythonic things like conversion between string types doesn't happen automatically, so that strings passed to protobuf routines have to be byte strings, not unicode strings

@david4096 david4096 self-requested a review April 3, 2017 22:30
Copy link
Member

@david4096 david4096 left a comment

Choose a reason for hiding this comment

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

Awesome thanks!

I'm wondering what this test earns us at this level. It seems that since there is a shared dependency on the schema we can isolate this test there?

Thanks for doing this!

@ljdursi
Copy link
Author

ljdursi commented Apr 4, 2017

Ooh, yeah, I didn't intend to add any tests here, hope I didn't accidentally include something from a different branch. Which test is the issue? I had to add some stuff to TestClientArguments to get it to work with the C++ implementation, but that had been there before.

@@ -0,0 +1,6 @@
package fakeobj;
Copy link
Member

Choose a reason for hiding this comment

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

This

extension_scope=None
)
], nested_types=[], enum_types=[], extensions=[])

def makeFakeObject(self):
Copy link
Member

Choose a reason for hiding this comment

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

this

Copy link
Author

Choose a reason for hiding this comment

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

Ah, yeah - that's the same as https://github.com/ljdursi/ga4gh-client/blob/master/tests/unit/test_cli.py#L430 , but with the stricter C++ implementation it actually needs to be a .proto file that gets compiled into a _pb2.py .

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants