Skip to content

Conversation

@westover
Copy link

Like #234 and #133 I am also interested in unifying python2/3 support so I have taken the opposite approach. Take python3 implementation and back port it to python2. This now supports python2 and 3 which is verifiable with Tox. I have added support for tox which is optional but viable. I have revived the lowercase method parse in schema.py to bring full backwards compatibility

So now recommended testing path for https://cwiki.apache.org/confluence/display/AVRO/How+To+Contribute is

cd lang/py3
tox

It would be advisable to follow up this pull with the creation of a symlink to lang/py so both packages can be created.

I hope this can help others like it will help me.

@westover
Copy link
Author

westover commented Jan 8, 2018

@krschultz @dcreager @ept @busbey @theturtle32 Can anyone provide a review on this?

@theturtle32
Copy link
Contributor

I don't know Python, so probably not.

@Vygo
Copy link

Vygo commented Jun 4, 2018

Are there plans on getting this in?

@iemejia iemejia added Python C JS Java Pull Requests for Java binding and removed Java Pull Requests for Java binding C JS labels Nov 29, 2018
@aaronchall
Copy link

Good grief - why capitalize the parse function (we don't capitalize functions in Python) and break users of the old py version when they try to convert to Python 3?

pacbbbbot pushed a commit to PacificBiosciences/pbcommand that referenced this pull request Apr 4, 2019
* apache/avro#251

> Good grief - why capitalize the parse function (we don't capitalize
> functions in Python) and break users of the old py version when they
> try to convert to Python 3?
@@ -0,0 +1,8 @@
[tox]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not include tox. Personally, I strongly feel that tox doesn't add any value anymore. It is from the pre-Docker time in which it was hard to isolate Python versions.

Copy link
Author

Choose a reason for hiding this comment

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

I used Tox because it is still simpler than changing all the tooling to support docker

Copy link
Contributor

Choose a reason for hiding this comment

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

There is already support for Docker. Having Tox inside of docker does not make any sense since they are both for isolating the application from the system.

@kojiromike
Copy link
Contributor

My concern with this approach is that the python3 implementation itself seems to be incomplete. Maybe it's just cruft in the lang/py implementation, but things like lang/py3/avro/txipc.py aren't functional right now. So I think we need to have a larger conversation about whether py3 has all the bits we want in it.

@westover
Copy link
Author

My concern with this approach is that the python3 implementation itself seems to be incomplete. Maybe it's just cruft in the lang/py implementation, but things like lang/py3/avro/txipc.py aren't functional right now. So I think we need to have a larger conversation about whether py3 has all the bits we want in it.

I agree. My push here was more around having a single dependency for either python2 or 3 with the assumption that the py3 implementation was already complete

@justinfx
Copy link

Is there any movement on this? I just started consuming avro for a project that supports both py2/3 and found it surprising that the py3 flavour changes capatalization. It would be really great if it used the same api and also followed common naming style, which means having parse()and not Parse(). Thanks!

@kojiromike
Copy link
Contributor

@justinfx not addressing this PR at the moment, but I agree with the parse/Parse conflict. I've created AVRO-2578 and #666 to address it.

@justinfx
Copy link

justinfx commented Jan 4, 2020

Question about how this will be released... Does this PR cause py2 user facing string values to become unicode objects in any cases? And do any of the apis calls now require a Unicode type as opposed to just any basestring? If so, will this be released as major version 2 to account for potentially breaking changes?

@kojiromike
Copy link
Contributor

I’ll respond in #744

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants