valkey-io / valkey-py

Valkey Python client based on a fork of redis-py
MIT License
21 stars 2 forks source link

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

Open kjaymiller opened 4 days ago

kjaymiller commented 4 days ago

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:

aiven-sal commented 4 days ago

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.