AVRO-1788: Implement Python 2 API #133
Conversation
spacharya
left a comment
spacharya
left a comment
There was a problem hiding this comment.
Looks good to me.
Just one nit
| return Names(names=self._names, default_namespace=namespace) | ||
|
|
||
| def GetName(self, name, namespace=None): | ||
| def _get_name(self, name, namespace=None): |
There was a problem hiding this comment.
this might break the usage of GetName in exisitng usage.
Not sure if we would want to make the function an internal use only.
There was a problem hiding this comment.
I wasn't 100% sure on this one, because get_name is already a function. When I did a usage search for GetName, the usages looked to be all within this file. When I searched for get_name, it seemed like other modules were using it.
Being that the existing get_name function delegates to this one, I think the functionality is still available for external use.
Does that seem appropriate to you? What are your thoughts?
There was a problem hiding this comment.
I am a bit conflicted on this. I see that this is the only usage, but I am unsure how people use it.
@rdblue can you check this out and let us know if moving a function as an internal use only in a minor is okay.
Else, +1
There was a problem hiding this comment.
If I understand correctly, convention is for downstream users to consider methods that do not start with _ to be part of the public API and safe to use. But, all of the renames in the PR are breaking changes so we could just bump the version to indicate what happened.
|
Hi, are there any updates on this PR? |
|
@ssaamm, how different are the py and py3 implementations? Is this something we could address by updating the python version to work with py3? |
|
I think unifying the From a cursory |
|
Are they mostly similar right now? What is the main difference between the existing implementations? |
|
One solution i can think of is creating methods with the same name. Throwing a warning message saying this has been deprecated and use the latest method. Then call the new method. For example, this is what pyopenSSL does if your binding is older. |
|
@rdblue, As a spot check, I've looked through a diff of Do any current maintainers have any insight into why the Python 2 and 3 implementations are separate? |
|
@ssaamm what can be possibly done to get this PR merged or get it reviewed by the maintainers ? |
|
Unsure. Perhaps @spacharya or @rdblue could comment on what next steps would be? |
|
I'm reluctant to move forward with this because it makes breaking changes to the python3 API without fixing the underlying problem -- that we have two python APIs. I'd much rather make py work with python 3 and remove py3 entirely. Another option is to add both APIs to py3 and deprecate py, but that seems like more work in the long term to maintain both APIs moving forward. As it stands, I don't think we should change the py3 API to match the py API. |
|
Have you looked at using python-modernize on the Python 2 code? That's an automated code changing tool like 2to3, but it aims to produce code that works on both Python 3 and 2. You'll probably still need some manual changes afterwards, but it can deal with a lot of the time-consuming routine changes that need to be done. |
|
I concur with @takluyver , the python-modernize approach would solve this issue and keep a single codebase. |
|
I had a go at this in #234, but had trouble running the tests with Python 3. |
This also includes a bump for sha2 library. As it stands, the trait changed, meaning this will actually be a breaking change for anyone using the fingerprint functionality of `avro-rs`.
7 participants
The Python 2 API and the Python 3 API differ unnecessarily, making it more difficult to write code that supports both major Python versions. Additionally, a lot of these function names were non-Pythonic.
This contribution is my original work, and I license the work to the project under the project's open source license.
Tests pass: