-
Notifications
You must be signed in to change notification settings - Fork 103
fix version import #583
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
fix version import #583
Conversation
joe-clickhouse
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.
Thanks @wiese-m. Appreciate the PR. See my comment below about common. As for the return type, that's a good call. Though there are some other functions in common that could benefit from return types as well. So if you're up for it, I'd suggest adding correct return types to the rest of the functions in common if they're missing them!
I also recently found a default/typing-related bug tracked in #586. All that's required there is changing the type of executor_threads parameter in create_async_client from Optional[int] to just a plain int and then change the default value from None to 0.
I see you've opened #582 as well (thanks!). So if you wouldn't mind, I suggest combing all this into a single PR that:
- Adds return types to all the functions missing them in common.py
- Fixes the default value and type of
executor_threadsfrom #586 - Fixes the test path you issue you found in #582
- Reverts the
versionimport in palaytest.py and uses the public entry point ofclickhosue_connect.version()
Let me know if you have any questions and thanks again!
|
Thanks for answer & suggestions. I pushed all the necessary changes. |
joe-clickhouse
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.
Looks good! Thanks for the help.
* fix version import * add missing type * add missing types * fix ClickHouse#586: inconsistent type hint * fix tls test path in contribution docs * revert playtest
* add executor parameter to AsyncClient * add note to change log * fix: we only want to shutdown the executor if we created it * add a custom object to represent the sentinel * fix typing * add warning log when executor_threads and executor are used * fix ambiguous time bug (#587) * fix version import (#583) * fix version import * add missing type * add missing types * fix #586: inconsistent type hint * fix tls test path in contribution docs * revert playtest --------- Signed-off-by: Meir <[email protected]> Co-authored-by: Joe S <[email protected]> Co-authored-by: Marek Wiese <[email protected]>
Summary
Fixed version import in
playtest.py& added missing return type inversion()Closes #586
Closes #582