TudorZgimbau
commented
1 year ago While this is probably for the better and definitely an improvement, I want to point out why those things were like that in the first place:
- Context managers in general are a good practice that might often get ignored because of the added syntax. A number of async database drivers suggest using the approach in their examples and aiohttp even explains why is the syntax required: https://docs.aiohttp.org/en/stable/http_request_lifecycle.html#aiohttp-request-lifecycle . I am, by no means, saying we should force users to use
async with, I'm just pointing out my initial thought process. - I thought about the test suit long before the refactoring. Didn't find a straight up good reason to not reuse a client, but it's probably better this way (it was easier to change config too, but that could be replaced with some sort of generator function).
- At that time, I believe the
closemethod wasn't necessary.
Another point that I need to make is, if you are concerned about the test isolation, please don't use a command from the SDK directly in order to test other. It's what I was describing in this pr: https://github.com/upstash/redis-python/pull/16 .
I also liked having a separate test folder for the formatters, but it's just a taste decision.
We had an implementation of the AsyncRedis that is not usable without the
async withblock. Not all users are going to use it that way, we should allow usages without it as well.I also found the way the tests written not ideal. We had a single sync and async client and reuse that throughout the test suite. I believe the proper way is to use fixtures per tests and make sure that the clients are closed no matter the result of the test. That resulted in a huge change, but I believe it was worth it. I believe the tests are a bit better right now, as we now have proper test isolation.
Also, the close method was missing from the AsyncRedis, so I added it as well.