Skip to content

Python flatbuffers read support#110

Closed
shaxbee wants to merge 11 commits intogoogle:masterfrom
shaxbee:python_support_v2
Closed

Python flatbuffers read support#110
shaxbee wants to merge 11 commits intogoogle:masterfrom
shaxbee:python_support_v2

Conversation

@shaxbee
Copy link
Contributor

@shaxbee shaxbee commented Dec 10, 2014

Python code generator for flatbuffers reading.

Wouter van Oortmerssen and others added 6 commits December 9, 2014 10:54
Change-Id: Ibc2bd88a636f3b4abf82a7c2722fc1e354dab848
Tested: on Linux.
Change-Id: I6ee09cf1e86a41b73bb3aa79b68871afb1a4e34f
Change-Id: I9787ab88e5bd4846d92995e2bb05d0c2121113ca
Tested: on Linux.
Change-Id: I458249d95e6d65ac039e84d947d2fdf4fd1c3809
Tested: on Linux.
Change-Id: Ie62096f7337a476bee7a6d46d652e594fb3124d2
Tested: on Linux.
Bug: 18201051
Change-Id: Ie187065698dfb6ba9d989e9d2c48bdd7cb870e89
@ghost
Copy link

ghost commented Dec 10, 2014

Thanks! Will have a look later.

@rw
Copy link
Contributor

rw commented Dec 12, 2014

Interesting. I'd like to see much more testing on this. Have you looked at the read tests in the C++ version? Fuzz-testing would be appropriate here.

Keep it up!

@shaxbee
Copy link
Contributor Author

shaxbee commented Dec 12, 2014

Sure. I'll be able to do that only after new year though.

On Fri, 12 Dec 2014 15:29 Robert notifications@github.com wrote:

Interesting. I'd like to see much more testing on this. Have you looked at
the read tests in the C++ version? Fuzz-testing would be appropriate here.

Keep it up!


Reply to this email directly or view it on GitHub
#110 (comment).

@shaxbee
Copy link
Contributor Author

shaxbee commented Dec 22, 2014

@rw: Working on fuzz tests now - they require builder support for tables / scalar fields, I've aligned Python builder interface with C++/Go as seen in 747030d

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a small thing, we follow the Google C++ style guide
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html

To adhere to this, you'll need to line up the arguments followed by the function definition to the opening parenthesis.

BTW: I tried pasting a formatted version in here but failed :(

Copy link
Contributor

Choose a reason for hiding this comment

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

   extern bool GeneratePython(const Parser &parser,
                              const std::string &path,
                              const std::string &file_name,
                              const GeneratorOptions &opts);

Copy link
Contributor

Choose a reason for hiding this comment

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

Why use a tuple rather than declare the members of this class via the init() method? Seems a little bit odd to me at the moment.

I have similar reservations about Struct and Table.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Immutability and speed of property access - same trick is being used in collections.namedtuple.
You can find more details about that here: of http://shaxbee.github.io/python-binary-part2-struct-unpack/

@rw
Copy link
Contributor

rw commented Dec 23, 2014

Please compare with the Python 2/3 port I pushed today: #112. Maybe we can combine these?

Copy link
Contributor

Choose a reason for hiding this comment

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

Please limit lines to 80 characters

@shaxbee
Copy link
Contributor Author

shaxbee commented Jan 2, 2015

Fuzz testing, fixed formatting, added more docstrings. I'll work on builder codegen now.

@shaxbee
Copy link
Contributor Author

shaxbee commented Apr 13, 2015

Hello, go ahead and close it, I'll work on struct based read/write and some other perf optimizations once #112 is merged.

@ghost ghost closed this Apr 13, 2015
This pull request was closed.
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.

5 participants