Typing support #1493
Typing support #1493
Conversation
|
|
| @@ -0,0 +1,339 @@ | |||
| from concurrent.futures import Future | |||
There was a problem hiding this comment.
This file is mostly done by stubgen, with some edits where stuff called things and it didn't match up. Note the heavy use of object as a "we don't know what this type is" marker, which mostly should make mypy whinge when it needs more type info and so help us fix it (v.s. using Any)
|
I'm also seeing unrelated build failures which is always fun :) |
Ah, no wait, my fault. 59f4069 fixes the problem by not using a pyproject.toml as that made |
|
@palfrey Will you continue this PR? :) |
Potentially. This is blocked on avro releasing a new version with the changes this needs and ideally on black getting integrated (as was being done in #1352 and then seems to be abandoned). If those get sorted, I can then go through and fix the issues here, but there's no point in repeatedly fixing all the things until then. |
|
And the Python 2 issues getting resolved, ideally by it getting dropped |
|
@palfrey apache/avro#1952 seems to be merged. |
But no release (https://github.com/apache/avro/releases) since last August |
src/confluent_kafka/avro/__init__.py
Outdated
| @@ -108,13 +107,13 @@ def produce(self, **kwargs): | |||
| else: | |||
| raise ValueSerializerError("Avro schema required for values") | |||
|
|
|||
| if key is not None: | |||
| if key is not None: | |||
There was a problem hiding this comment.
| if key is not None: | |
| if key is not None: |
There was a problem hiding this comment.
There's a lot of issues like this, and I'd mostly filed them under "get black fixed". See #1352 and #1493 (comment)
There was a problem hiding this comment.
Ah yes makes sense. Unfortunately maintainers don't seem pretty active
There was a problem hiding this comment.
Sadly, yes. Ironically, I'm job hunting at the moment, and had applied to Confluent but they were looking mostly for Java devs IIRC :( If they wanted to employ me to fix these things that might speed things up a lot...
There was a problem hiding this comment.
If they wanted to employ me to fix these things that might speed things up a lot...
Oh please ! This project is half-dead and Confluent is a commercial entity.
just FYI avro finally published a release 2 days ago! |
|
Is this PR going to be merged soon? Is there any blocker or changes left before merging this? |
Didn't know there was a CLA and TBH I wouldn't have done this if I'd known about it, but signed now. Waiting on review. |
|
cc @jkuhnashconfluent @emasab @pranavrth for review since you three were on the last PRs to be merged into this repo. |
|
Hey Guys, Sorry for the late reply. We couldn't pay much attention to this PR. This is an important PR which we would like to incorporate in our client. I will try to get some time in coming weeks for reviewing this PR. |
How's that going? |
|
It's a shame that all of this work has been done and the "coming weeks" has slowly turned into 4 and a half months. I fear that if this sits much longer, this contribution will start to grow stale and require additional work to get in a state that it can be merged again. I think having type hints is exceptionally valuable not just for the maintenance of this project but also the maintenance of any projects that consume this library and I would love to see merging this PR find its way back onto Confluent Inc's list of priorities. |
I'm mostly willing to do the work to un-stale it, but only when I know there will actually be review and possible merge of it, and I'm otherwise ignoring conflicts until that point. If I'd seen a first round of review that at least looked at what was done so far (and ideally also addressed the Python versioning and black issues) then I'd come back to this, but so far I'm seeing no indications of this actually occuring any time soon despite prior comments on this PR. |
|
@palfrey have you considered moving the effort to Typeshed? I don't know what it entails but if I understand correctly it will actually have a chance to be merged 😔 |
I hadn't. It's a fair amount of work (ripping out the types from the individual .py files and redoing as just the signatures in .pyi files), and TBH, I'm not exactly enthused about that, especially as I don't use the library currently. If this comes back to life, I'm willing to fix things here though. OTOH, if anyone else wants to take what I've done here and do that work in Typeshed, awesome. Some level of credit/mention would be appreciated, but otherwise, go for it! |
I think it should be possible to automate this with stubgen. |
Fixes #1408
This PR does at least an initial pass of adding typing support. Please note it does not support Python 2 at the moment (although that might be getting dropped?) or before 3.5. OTOH, given Python <3.7 is EoL, I don't think they should be supported, but you might want take a different approach, which would need conversion to comment-based typing. This is doable, but a chunk more work and wanted to check if that's needed.
I'd also note that my usual approach on these things is "use the existing CI support for all the version testing" but that doesn't appear to be doable here as it's not setup for that? Relatedly, my formatting is a little wonky, which usually I'd sort out with black, which appears to have been kinda setup and I'd love to get that resolved before this got merged.
It also won't fully work until Avro releases a new version with typing support. Note that this won't change the runtime support requirements, only the requirements if you want to do type checking.Any thoughts?