Python 3 support #7
Conversation
| finally: | ||
| if newsubs: | ||
| free(newsubs); | ||
| pass |
There was a problem hiding this comment.
what's the story with removing this function?
There was a problem hiding this comment.
AFAICT subAll isn't used in PyZephyr itself anywhere, and based on the name I'm assuming that nothing in _zephyr is intended to be "public" (except for receive and ZNotice, of course, which are imported into zephyr's namespace). subAll didn't build (I think due to the byte/string mismatch), so I just removed it rather than trying to fix it. I could fix it instead.
There was a problem hiding this comment.
I think we'll want to fix it; it's used by the Zulip/zephyr mirroring system.
There was a problem hiding this comment.
Ah, good to know. Does it make sense to expose a bulk subscription interface through the Subscriptions class, then, so that API clients don't need to use _zephyr? (What's the motivation for using subAll rather than calling Subscriptions.add?)
There was a problem hiding this comment.
Yep, it's to do a bulk subscribe. Important for perf when you're subscribing to thousands of classes.
|
Yeah, the first two are more solid. The third doesn't actually finish Python 3 compatibility, and depending on preferences for how to handle the byte/string boundary, it may not be particularly close to final form. Which gets back to the question of how we'd rather handle it. I think I'd prefer to interface with PyZephyr with strings and have it handle the conversions, but I'm not sure how reasonable that is. (I also think that might involve enough futzing with encodings to prefer to move some of the logic out of Cython and into actual Python...) |
| @@ -29,7 +35,6 @@ def __new__(cls): | |||
|
|
|||
| def __del__(self): | |||
| _z.cancelSubs() | |||
| super(Subscriptions, self).__del__() | |||
There was a problem hiding this comment.
Hm, I removed this based on the last answer from http://stackoverflow.com/questions/36722390/python-3-super-del, but judging by https://docs.python.org/3.3/reference/datamodel.html#object.__del__, it looks like we really are supposed to call it. (Not that not calling it has obviously broken yet.)
There was a problem hiding this comment.
Yeah, this should stay; Python does not automatically call __del__ on the superclass.
|
|
||
| if is_py3: | ||
| item = [bytes(i, "utf-8") for i in item] | ||
| print(item) |
| @@ -29,7 +35,6 @@ def __new__(cls): | |||
|
|
|||
| def __del__(self): | |||
| _z.cancelSubs() | |||
| super(Subscriptions, self).__del__() | |||
There was a problem hiding this comment.
Yeah, this should stay; Python does not automatically call __del__ on the superclass.
|
|
||
| if is_py3: | ||
| item = [bytes(i, "utf-8") for i in item] |
There was a problem hiding this comment.
This certainly shouldn’t be conditionally encoded; that makes the API different for different Python versions. The Python 3 version is wrong here, because you’re encoding not just the string passed to libzephyr, but also the string added to the Python set. One would expect that it would at least be possible (i.e. for a tuple that’s already in canonical format) for item in subscriptions to hold after subscriptions.add(item).
|
So, any thoughts on whether the API should be unicode or bytes? I figure I'll hold off on the cleanup until I have something that works, which is blocked on figuring out what the API should look like. |
|
I don't know if there's any thinking about converting PyZephyr to Python 3? It's conceivable I could work on this again, but I'm still hoping for guidance on
before I proceed. |
|
Has anybody looked at Python 3 recently? I think my inclination was:
scripts.mit.edu is working on an upgrade, for which we might care about Python 3. (Chiron would also certainly like Python 3.) |
|
I'm not aware of any work on it
…-Tim Abbott (mobile)
On Mon, Dec 9, 2019, 2:21 PM Alex Dehnert ***@***.***> wrote:
Has anybody looked at Python 3 recently?
I think my inclination was:
- zephyr should support strings everywhere, and possibly accept bytes
too (even for Py2, returning unicode strings should be fine, right? and I
don't know if it's reasonable to go straight from a version that's Py2-only
to Py3-only, given the state of upstream Py2 support)
- _zephyr should be private, with whatever API is easiest to make work
in C
- Anything that _zephyr has that clients commonly want (eg, subAll)
should be exported to the zephyr module itself
scripts.mit.edu is working on an upgrade, for which we might care about
Python 3. (Chiron would also certainly like Python 3.)
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#7?email_source=notifications&email_token=AAU6NWRRTBFVPOZCEEAP5ALQX25HTA5CNFSM4C4RKUR2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGK3B2Q#issuecomment-563458282>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAU6NWQ4R7CGBIVY3T52RATQX25HTANCNFSM4C4RKURQ>
.
|
|
Please see my PR #8 which adds complete Python 3 support without any API changes. |
It would be nice to have Python 3 support in python-zephyr. Here's a start, though I don't think it's complete (and I haven't gone to any effort to clean it up yet). (I'd have submitted this as an issue instead of a pull request, but issues appear disabled.)
One decision that probably deserves other opinions is whether the library should expose strings or bytes, and whether to accept bytes, strings, or both. My inclination is that the library should expose strings for ~everything, but I'm not sure how reasonable that is (and I don't have much experience with Python 3).