Skip to content
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

implement a single validation function around the service_uri to reduce code/test complexity around connection URL Parsing #25

Closed
kjaymiller opened this issue Jun 27, 2024 · 1 comment · Fixed by #29
Assignees

Comments

@kjaymiller
Copy link
Contributor

Thanks for wanting to report an issue you've found in valkey-py. Please delete this text and fill in the template below.
It is of course not always possible to reduce your code to a small test case, but it's highly appreciated to have as much data as possible. Thank you!

Version: 5.1.0b7

Platform: Python 3.12 on MacOS ARM running OS Sonoma

Description:

In #23 I proposed including redis and rediss as valid protocols. I noticed while implementing the change that the code was almost identical with the main difference being variable names.

Diffs of code base showing that the code in both sync and async versions are the same

I propose that the protocol and url code be moved into a module that can be called in both places. This would reduce identical code reducing the amount of code needed to cover as well as making the testing for that portion of the code-base completely non-reliant on server setup and connection. This tests that the code does what it's supposed to do not that the code interacts with the platform how we expect it to (which should be a different and separate test in itself)

Proposed changes:

  • move parse_url to a new python module (or possibly refactor the aysnc connection to inherit from the synchronous one if possible)
  • create a dedicated test path for that parsing and move the identical tests in the sync/async versions of test_connection_pool.py.TestConnectionPoolURLParsing to that dedicated test module.
@kjaymiller kjaymiller changed the title single connection base object validation to reduce code/test complexity around connection URL Parsing implement a single validation function around the service_uri to reduce code/test complexity around connection URL Parsing Jun 27, 2024
@ahmedsobeh ahmedsobeh self-assigned this Jun 28, 2024
@aiven-sal
Copy link
Member

It definitely makes sense to share parse_url implementation across the 2 classes. And we should keep the "asyncio" version, because the other is actually broken (it checks the URI scheme case sensitively which is against RFC3986).
It would certainly make sense to test the "new" parse_url in a single place, but I would still keep the separate TestConnectionPoolURLParsing (at least part of them) because they don't just test parse_url.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants