-
Notifications
You must be signed in to change notification settings - Fork 14
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
making either store or config #30
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
import pytest | ||
import redislite | ||
from redis.client import Redis | ||
|
||
from pydantic_redis import Store, RedisConfig | ||
|
||
|
||
def test_need_config_or_redis(): | ||
with pytest.raises(ValueError): | ||
_ = Store(name="no-redis-or-config") | ||
|
||
|
||
def test_cannot_provide_both(unused_tcp_port: int): | ||
redis = redislite.Redis(host="localhost", port=unused_tcp_port) | ||
config = RedisConfig(host="localhost", port=unused_tcp_port) | ||
|
||
with pytest.raises(ValueError): | ||
_ = Store( | ||
name="redis-or-config", | ||
redis_config=config, | ||
redis_store=redis, | ||
) | ||
|
||
def test_config_creates_new(): | ||
store = Store( | ||
name="redis-or-config", | ||
redis_config=RedisConfig( | ||
host="localhost", | ||
port=6379, | ||
db=0, | ||
), | ||
) | ||
assert store.redis_store is not None | ||
assert isinstance(store.redis_store, Redis) | ||
|
||
|
||
def test_store_creates_new(unused_tcp_port: int): | ||
redis = redislite.Redis(host="localhost", port=unused_tcp_port) | ||
pydantic_redis_store = Store( | ||
name="redis-or-config", | ||
redis_store=redis, | ||
) | ||
|
||
assert pydantic_redis_store.redis_store == redis |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to reintroduce the ambiguity we removed when we removed the
redis_store
argument.For instance someone might set both redis_config and redis_store with different redis databases.
It is not quickly apparent to anyone that the
redis_store
has higher precedence over theredis_config
. This might cause that person a lot of frustration when they don't see the data where they expect.We need to have a single way of passing the redis instance so that there is never a possibility of passing two redis instances.
Options:
RedisConfig
to allow passing a pre-constructed redis instance.Con: It might require a lot of complicated code to ensure two instances are never passed to it (the
RedisConfig
).Pro: The signature of
AbstractStore
is untouched; no breaking change.redis_config
argument toredis_store: Union[Redis, AioRedis, RedisConfig]
.Con: The signature of
AbstractStore
is changed; this is a breaking change.Pro: The code is simple and ensures that only one redis instance is ever passed to the store.
Pro: The name
redis_store
as opposed toredis_config
makes intuitive sense for a parameter that can either be a redis instance or aRedisConfig
redis_config
argument toredis_config: Union[Redis, AioRedis, RedisConfig]
.Pro: The signature of
AbstractStore
is unchanged; no breaking change.Pro: The code is simple and ensures that only one redis instance is ever passed to the store.
Con: The name
redis_config
as opposed toredis_store
does not make it apparent it can either be a redis instance or aRedisConfig
especially since there is an actual class called "RedisConfig"...There could be other options but personally, I am leaning more on option 3 then adding some extra documentation specifying that
redis_config
can also receive a redis instance. This way, the many applications currently running out there that were built basing on our original documentation do not fail suddenly on upgrading.I also expect that not many people except the "power users" (as you commented earlier) will want to have a way of passing in their pre-constructed redis instance. And since they are "power users", they definitely can comb through the documentation for such finer control options :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to stick with the names you have:
redis_config
andredis_store
. The validator does this:So if a programmer happens to pass in values for both the
redis_config
and theredis_store
, they will be given aValueError
, explaining they can only pass in one of them.This way the parameter names remain descriptive.
I will add a docstring that explains this better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just added the following DocString:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validator is demonstrated to work in the test
test_cannot_provide_both
as follows:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need the library to be straight forward. Adding two ways of initializing the Store needs to be last resort.
For now, just extend the Store