Skip to content

Conversation

@feribg
Copy link
Contributor

@feribg feribg commented Apr 10, 2019

This is a very early implementation of a python stub generator. It's derived from the C# Java generator, so it follows most of the code and conventions adopted there.

The runPythonTests gradle task will build the python stubs in the project_root/python/tests/gen and run the unit tests. So far only car_example_schema.xml is implemented, but happy to port any other tests that focus on specific edge cases (Maybe a FIX sample).

Built and tested with python 3.7, but in practice any version that supports https://docs.python.org/3/c-api/buffer.html should work, so 2.7 should be fine too, although I didn't try it yet (also I much rather not offer support for it generally).

If you think it's not worth it adding and maintaining that code in the mainline I'm happy to fork off only the IR and Java generator as well and providing it as an unsupported 3rd party generator, depending on the discussion.

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 10, 2019

This PR fails the build. As a minimum it should pass all tests.

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 10, 2019

Build is still failing with lots of code style issues.

@feribg
Copy link
Contributor Author

feribg commented Apr 10, 2019

I installed the checkstyle idea plugin imported the style and reformatted everything, but i get tons of errors on indent and line length. Is that an idea issue not respecting fully the checkstyle config? I attached an example of what the IDE does in the second screenshot when reformat is applied with the checkstyle, but then gradle checkstyle reports that as incorrect.
I think the rest that was wrong in terms of temp folders, etc, should be resolved.

Reformatted:
Screen Shot 2019-04-10 at 2 37 40 PM
Original:
Screen Shot 2019-04-10 at 2 37 31 PM

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 10, 2019

IDEA does not properly support strict indenting.

@feribg
Copy link
Contributor Author

feribg commented Apr 10, 2019

Got it, okay it'll take me a while to clean them up manually then. I'll push a fix once the checkstyle passes. Meanwhile the rest should work (build, test, etc), without the style.

@feribg
Copy link
Contributor Author

feribg commented Apr 16, 2019

I think everything is more or less cleaned up, could use more unit tests, but will likely postpone for after initial feedback.

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 18, 2019

Looks reasonable. You probably want to clean up the copyright and C# references in Javadoc.

The major point for me is that is follows the combined encoder and decoder format of the C# generator and not the separation in the Java generator. We've seen this is a much better approach as the separate decoder can deal with read only buffers.

@feribg
Copy link
Contributor Author

feribg commented Apr 18, 2019

I tend to agree with that point. I already had the issue of having to use mutable buffer types. I'll have a look into how much effort it is to split them. If it turns out to be too big of a rewrite are you OK merging as is contingent on cleanup.

Are there any specific edge cases you can think of in general for which it would be a good idea to port the tests?

@mjpt777
Copy link
Contributor

mjpt777 commented Apr 18, 2019

Have a read back through the issues to get a feel for what to watch out for.

@feribg
Copy link
Contributor Author

feribg commented Apr 29, 2019

Added some new test cases, additional fixes, the cleanups you suggested and some fixes post rebase. Seems like one commit from Apr 9, diverged the example-schema.xml from the sbe-samples and sbe-tools/tests. Also I see that travis failed for openjdk 11 with an infrastructure error, but the error code doesn't tell me much.

@feribg
Copy link
Contributor Author

feribg commented May 14, 2019

Just wondering if you had a chance to take a peak at merging that?

@mjpt777
Copy link
Contributor

mjpt777 commented May 15, 2019

As mentioned above we want to move away from the use of a single codec for encode and decode rather than propagate this pattern. Can you look at doing something similar to the Java codec?

@feribg
Copy link
Contributor Author

feribg commented May 27, 2019

Yep I will try and put some cycles towards that, will give you an update when i have a cleaner prototype.

@feribg
Copy link
Contributor Author

feribg commented Aug 8, 2019

Sorry about the big delay here, got side tracked. This is fully reworked to have enc + dec pattern like the Java version. I used it as the base blueprint. Waiting for travis to complete, let me know how it looks like.

@feribg
Copy link
Contributor Author

feribg commented Sep 13, 2019

@mjpt777 Wondering if you're still considering merging this upstream or keep it as a fork for Py support? Thx!

@mjpt777 mjpt777 merged commit f44b39f into aeron-io:master Sep 16, 2019
@mjpt777
Copy link
Contributor

mjpt777 commented Sep 16, 2019

Sorry but I had to revert this. As I worked through the code refactoring it I noticed far to many warnings and have concerns about the completeness. Please resubmit with less warnings and try not to have unused methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants