-
Notifications
You must be signed in to change notification settings - Fork 934
Typing support #1493
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Typing support #1493
Conversation
CLA assistant check
All committers have signed the CLA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
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 pip install do things without the important include paths.
thanksyouall
commented
Mar 21, 2023
@palfrey Will you continue this PR? :)
@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
Eyal-Shalev
commented
May 23, 2023
@palfrey apache/avro#1952 seems to be merged.
@palfrey apache/avro#1952 seems to be merged.
But no release (https://github.com/apache/avro/releases) since last August
@Viicos
Viicos
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we fully parametrize generics with Any? e.g. Sequence[Any] instead of Sequence.
The styling standard regarding default values is to add spaces around the = char: arg: Optional[str] = None
src/confluent_kafka/avro/__init__.py
Outdated
@Viicos
Viicos
May 29, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a lot of issues like this, and I'd mostly filed them under "get black fixed". See #1352 and #1493 (comment)
@Viicos
Viicos
May 29, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes makes sense. Unfortunately maintainers don't seem pretty active
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
@joaoe
joaoe
Jun 21, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
cameronbrill
commented
Jul 12, 2023
@palfrey apache/avro#1952 seems to be merged.
But no release (apache/avro/releases) since last August
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?
@palfrey Do you need to sign the Contributor License Agreement?
Is this PR going to be merged soon? Is there any blocker or changes left before merging this? @palfrey Do you need to sign the Contributor License Agreement?
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.
cameronbrill
commented
Sep 26, 2023
cc @jkuhnashconfluent @emasab @pranavrth for review since you three were on the last PRs to be merged into this repo.
hantonelli
commented
Oct 3, 2023
Viicos
commented
Oct 3, 2023
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.
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?
lannuttia
commented
Feb 29, 2024
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 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'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.
Eyal-Shalev
commented
Mar 11, 2024
@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 😔
@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!
Barabazs
commented
Mar 13, 2024
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)
I think it should be possible to automate this with stubgen.
@rayokota I'm seeing various work on this repository from you. Any interest in this PR?
EliiseS
commented
Apr 8, 2025
Any updates on this? Typings would be nice to have :)
jalelegenda
commented
Apr 18, 2025
bump
Uh oh!
There was an error while loading. Please reload this page.
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?