Skip to content

typing for client __init__ #3357

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 8 commits into from
Feb 10, 2025
Merged

Conversation

Vulwsztyn
Copy link
Contributor

@Vulwsztyn Vulwsztyn commented Aug 19, 2024

Pull Request check-list

Please make sure to review and check all of these items:

  • Do tests and lints pass with this change?
  • Do the CI tests pass with this change (enable it first in your forked repo and wait for the github action build to finish)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?
  • Is there an example added to the examples folder (if applicable)?
  • Was the change added to CHANGES file?

NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.

Description of change

I only added typings to __init__ method of client:Redis class

I realise this exists: #3252

But there were some comments and it is dead since the end of May, while I hope this PR introduces so little change that it is instantly mergeable.

@vladvildanov
Copy link
Collaborator

@Vulwsztyn Tests are failing, cause of conditional OpenSSL dependency

@Vulwsztyn
Copy link
Contributor Author

@vladvildanov thanks I'll make it unconditional

@vladvildanov
Copy link
Collaborator

@Vulwsztyn Thank you for your contribution!

@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch 3 times, most recently from 3771028 to d6a4326 Compare September 4, 2024 09:23
@Vulwsztyn
Copy link
Contributor Author

@vladvildanov I changed the typing to string literals so that it doesn't require imports and removed the conditional imports

@Vulwsztyn
Copy link
Contributor Author

@vladvildanov Any insight as to why the tests are failing now? Should I do something?

@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch from 852adb5 to c53e5e3 Compare September 11, 2024 16:08
@Vulwsztyn
Copy link
Contributor Author

Vulwsztyn commented Sep 11, 2024

@vladvildanov I fixed the linting issues. Sorry it took so long.

> invoke linters
All done! ✨ 🍰 ✨
134 files would be left unchanged.
Running flynt v.0.69
Running flynt in dry-run mode. No files will be changed.

Flynt run has finished. Stats:

Execution time:                            2.488s
Files modified:                            0

_-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_.
Please run your tests before committing. Did flynt get a perfect conversion? give it a star at: 
~ https://github.com/ikamensh/flynt ~
Thank you for using flynt. Upgrade more projects and recommend it to your colleagues!
> invoke linters
All done! ✨ 🍰 ✨
134 files would be left unchanged.
Running flynt v.0.69
Running flynt in dry-run mode. No files will be changed.

Flynt run has finished. Stats:

Execution time:                            2.488s
Files modified:                            0

_-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_._-_.
Please run your tests before committing. Did flynt get a perfect conversion? give it a star at: 
~ https://github.com/ikamensh/flynt ~
Thank you for using flynt. Upgrade more projects and recommend it to your colleagues!

@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch from c53e5e3 to 7644229 Compare September 14, 2024 12:19
@Vulwsztyn
Copy link
Contributor Author

rebased unto master

@vladvildanov
Copy link
Collaborator

@Vulwsztyn OpenSSL is still an issue if you have a look in CI logs

@vladvildanov vladvildanov added the techdebt Things that can be improved or refactored label Sep 19, 2024
@Vulwsztyn
Copy link
Contributor Author

@vladvildanov Thanks for the ping.

I don't get something here. In this line

import OpenSSL
in this project we import OpenSSL, but it is not a dependency of this project? Should it remain so?

@vladvildanov
Copy link
Collaborator

@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch 2 times, most recently from f0339e7 to 4b76013 Compare September 20, 2024 17:22
@Vulwsztyn
Copy link
Contributor Author

Removed the typing requiring openssl

@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch from 4b76013 to 5ac186a Compare October 21, 2024 16:12
@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch from 5ac186a to 31dbb85 Compare November 12, 2024 13:37
@Vulwsztyn
Copy link
Contributor Author

Could you try running the tests please?

@Vulwsztyn Vulwsztyn force-pushed the typing_for_client_init branch from 336b259 to f5e9163 Compare February 4, 2025 12:30
@Vulwsztyn
Copy link
Contributor Author

I refixed lint

@Vulwsztyn
Copy link
Contributor Author

I see that all checks are passing. Would it be possible to merge this?

@vladvildanov
Copy link
Collaborator

@Vulwsztyn waiting for checks after latest sync with master and good to go

@vladvildanov vladvildanov merged commit 9eab6fe into redis:master Feb 10, 2025
35 checks passed
vladvildanov added a commit that referenced this pull request Feb 11, 2025
* typing for client __init__

* typing with string literals

* retry_on_error more specific typing

* retry typing

* fix lint

---------

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>
vladvildanov added a commit that referenced this pull request Feb 11, 2025
* Fixed flacky TokenManager test (#3468)

* Fixed flacky TokenManager test

* Fixed additional flacky test

* Removed token count assertion

* Skipped test on version 3.9

* Fix incorrect attribute reuse (#3456)

add CacheEntry

Co-authored-by: zhousheng06 <zhousheng06@meituan.com>
Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* Expand type for EncodedT (#3472)

As of PEP 688, type checkers will no longer implicitly consider bytearray to be compatible with bytes

* Moved self._lock initialisation to Pool constructor (#3473)

* Moved self._lock initialisation to Pool constructor

* Added test case

* Codestyle fixes

* Added correct annotations

* DOC-4423: add TCEs for various command pages (#3476)

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* DOC-4345 added testable JSON search examples for home page (#3407)

* DOC-4345 added testable JSON search examples for home page

* DOC-4345 avoid possible non-deterministic results in tests

* DOC-4345 close connection at end of example

* DOC-4345 remove unnecessary blank lines

* Adding unit text fixes to improve compatibility with MacOS. (#3486)

* Adding unit text fixes to improve compatibility with MacOS.

* Applying review comments

* Unifying the exception msg validation pattern for both test_connection.py  files

---------

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* Add return type to `close` functions (#3496)

* Add types to ConnectionPool.from_url (#3495)

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* Add types to execute method of pipelines (#3494)

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* DOC-4796 fixed capped lists example (#3493)

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* typing for client __init__ (#3357)

* typing for client __init__

* typing with string literals

* retry_on_error more specific typing

* retry typing

* fix lint

---------

Co-authored-by: Vladyslav Vildanov <117659936+vladvildanov@users.noreply.github.com>

* test: Updated CredentialProvider test infrastructure (#3502)

* test: Updated CredentialProvider test infrastructure

* Added linter exclusion

* Updated dev dependency

* Codestyle fixes

* Updated async test infra

* Added missing constant

* Updated package version

* Updated testing versions and docs

* Updated server versions

* Fixed test

---------

Co-authored-by: zs-neo <48560952+zs-neo@users.noreply.github.com>
Co-authored-by: zhousheng06 <zhousheng06@meituan.com>
Co-authored-by: Shantanu <12621235+hauntsaninja@users.noreply.github.com>
Co-authored-by: David Dougherty <dwdougherty@gmail.com>
Co-authored-by: andy-stark-redis <164213578+andy-stark-redis@users.noreply.github.com>
Co-authored-by: petyaslavova <petya.slavova@redis.com>
Co-authored-by: Patrick Arminio <patrick.arminio@gmail.com>
Co-authored-by: Artur Mostowski <artur.mostowski@protonmail.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
techdebt Things that can be improved or refactored
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants