Skip to content

Conversation

@jonathan-h-grebe
Copy link
Contributor

@jonathan-h-grebe jonathan-h-grebe commented Jun 19, 2024

solves #12149, the gist of which is:

  • a Redis client (redis.Redis) can be obtained by using either its constructor, or from_url method
  • connection options can be specified as arguments to either
  • some of those options (eg ssl_ca_data) were missing as defined parameters for from_url
  • this caused mypy to give false positives those missing SSL options were specified as kwargs to from_url

Change Details

To fix the issue, following 9 options were added as params to from_url

  • ssl_ca_path
  • ssl_ca_data
  • ssl_password
  • ssl_validate_ocsp
  • ssl_validate_ocsp_stapled
  • ssl_ocsp_context
  • ssl_ocsp_expected_cert
  • redis_connect_func
  • credential_provider

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@jonathan-h-grebe jonathan-h-grebe marked this pull request as ready for review June 19, 2024 13:59
@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@JelleZijlstra
Copy link
Member

Thanks! This becomes a bit repetitive, maybe we should use Unpack[] with a TypedDict (PEP-692) to define these kwargs?

@jonathan-h-grebe
Copy link
Contributor Author

Thanks! This becomes a bit repetitive, maybe we should use Unpack[] with a TypedDict (PEP-692) to define these kwargs?

Good point - those (almost) identical parameters being repeated 5 times do seem a bit much.

However, as TypedDicts don't support default values for keys, it wouldn't be possible to define the parameter defaults.
We could get around that by defining a dataclass, but I think that would quite an impact on existing code bases, having to switch from dict to class based syntax.

@JelleZijlstra JelleZijlstra merged commit 735942a into python:main Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants