appnexus / pyrobuf

A Cython alternative to Google's Python Protobuf library
Other
554 stars 76 forks source link

add parser support for oneof and map fields. #122

Closed AlmogCohen closed 5 years ago

AlmogCohen commented 5 years ago

I'm mostly interested in the parser and not in the C extensions generated so I've improved the parser while keeping its default behaviour intact. I'm not a C guy so nothing I can help with adding the complete support for the new fields. I do hope it might help folks down the line add the full support.

Commit includes:

AlmogCohen commented 5 years ago

I couldn't get the tests to work locally following the instructions as all unit tests error out. I'm convinced that none of the tests were broken as I've changed nothing in the existing parser's design. I've also tested parsing of some files I work with before and after the change. I'd be happy to run the tests again if proper instructions will be given. The errors look like that:

    def test_sint64(proto_lib):
>       from test_signed_integer_proto import Int
E       ModuleNotFoundError: No module named 'test_signed_integer_proto'
AlmogCohen commented 5 years ago

Might be relevant for https://github.com/appnexus/pyrobuf/issues/109 and https://github.com/appnexus/pyrobuf/issues/95 if someone would like to implement the C side of map and oneof.

Edit: Actually there is also a slight improvement for existing users as now they will get better exceptions when trying to parse the disabled oneof and map fields rather than getting some bogus messages.

AlmogCohen commented 5 years ago

P.S. It would be nice to have the C side of the new fields implemented even though I'm not in need for the speed. I'm only looking for a convenient API for working with .proto definitions in python. I'm thinking maybe writing something to translate .proto file into schematics python classes.

tburmeister commented 5 years ago

P.S. It would be nice to have the C side of the new fields implemented even though I'm not in need for the speed. I'm only looking for a convenient API for working with .proto definitions in python. I'm thinking maybe writing something to translate .proto file into schematics python classes.

Agreed - hopefully I will get to it someday but realistically it won't happen until I or someone willing to implement it needs the functionality.

tburmeister commented 5 years ago

I couldn't get the tests to work locally following the instructions as all unit tests error out. I'm convinced that none of the tests were broken as I've changed nothing in the existing parser's design. I've also tested parsing of some files I work with before and after the change. I'd be happy to run the tests again if proper instructions will be given. The errors look like that:

    def test_sint64(proto_lib):
>       from test_signed_integer_proto import Int
E       ModuleNotFoundError: No module named 'test_signed_integer_proto'

It's trying to import a built protobuf spec - you first need to build and install all the test message specs.

tburmeister commented 5 years ago

I couldn't get the tests to work locally following the instructions as all unit tests error out. I'm convinced that none of the tests were broken as I've changed nothing in the existing parser's design. I've also tested parsing of some files I work with before and after the change. I'd be happy to run the tests again if proper instructions will be given. The errors look like that:

    def test_sint64(proto_lib):
>       from test_signed_integer_proto import Int
E       ModuleNotFoundError: No module named 'test_signed_integer_proto'

It's trying to import a built protobuf spec - you first need to build and install all the test message specs.

Try running pyrobuf --install tests/proto/ before running the tests. The instructions for testing are not clear and testing should be simplified - I will try to make that happen - thanks for bringing this to light.

tburmeister commented 5 years ago

It would be great if we could add a test message spec to demonstrate that this works.

tburmeister commented 5 years ago

I checked out your branch locally and all tests pass for me.

AlmogCohen commented 5 years ago

I couldn't get the tests to work locally following the instructions as all unit tests error out. I'm convinced that none of the tests were broken as I've changed nothing in the existing parser's design. I've also tested parsing of some files I work with before and after the change. I'd be happy to run the tests again if proper instructions will be given. The errors look like that:

    def test_sint64(proto_lib):
>       from test_signed_integer_proto import Int
E       ModuleNotFoundError: No module named 'test_signed_integer_proto'

It's trying to import a built protobuf spec - you first need to build and install all the test message specs.

Try running pyrobuf --install tests/proto/ before running the tests. The instructions for testing are not clear and testing should be simplified - I will try to make that happen - thanks for bringing this to light.

I've added that to the README.md and created a small TESTING section there. Now the tests do run locally :)

AlmogCohen commented 5 years ago

I've added decent unittests for both oneof and MapField parsing capabilities. I've also grouped all parser unittests grouped under test_parser folder. One of the benefits of doing so is allowing someone to run the pure python unittests without the need to build the C extensions. (pytest allow folder-specifc runs)

On another note, following your instructions I've successfully run the unittests but the second run failed with another error. Not a big deal for this PR but it would be nice to fix the issue though.

ImportError while importing test module '/Users/almogcohen/Clients/algosec/code/pyrobuf/tests/test_unicode_strings.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
tests/test_unicode_strings.py:5: in <module>
    from proto_lib_fixture import proto_lib
E   ModuleNotFoundError: No module named 'proto_lib_fixture'

Thank you much for the code review and quick comments!

tburmeister commented 5 years ago

This is really great! Thanks so much for doing this! Let me know when you're ready and I can merge.

AlmogCohen commented 5 years ago

flake8 added, all warnings fixed in the file I worked on. Just wanted to say that in my projects I try to stick to line length of 120 and not 80. 80 feels to me like a screen size from another era ;) Anyway I go with the flow...

With the new line-length enforcement there are a few more of the function definitions with the style you didn't like but I couldn't think of any other way!

Ready for a merge 😎

tburmeister commented 5 years ago

Great! Thanks again! 120 probably makes more sense... Maybe I'll change it at some point.

AlmogCohen commented 5 years ago

Awesome thank you so much! :)