-
Notifications
You must be signed in to change notification settings - Fork 46
Setting client lib name properly by wrapping multiplxer creation #420
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
base: master
Are you sure you want to change the base?
Conversation
hey @slorello89 @uglide |
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.
Couple minor nits, overall LGTM 👍 - will need to update docs to reflect it as well.
src/NRedisStack/RedisClient.cs
Outdated
/// Represents a Redis client that can connect to a Redis server | ||
/// providing access to <see cref="IRedisDatabase"/> instances as well as the underlying multiplexer. | ||
/// </summary> | ||
public class RedisClient |
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 should be lifted out into an interface IRedisClient
and should take an IConnectionMultiplexer
in all the places.
src/NRedisStack/RedisClient.cs
Outdated
/// <summary> | ||
/// Creates a new <see cref="RedisClient"/> instance. | ||
/// </summary> | ||
/// <param name="configuration">The string configuration to use for this client.</param> |
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.
Link back to docs
|
||
namespace NRedisStack.RedisStackCommands | ||
{ | ||
internal class DatabaseWrapper : IDatabase |
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'm assuming the idea here is that if we have a different opinion of how a command should be handled than StackExchange.Redis we can override it here without asking them to break?
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.
My main motivation here is more on keeping all commands accessible from same object/reference and keep the usage exact same compared to how it is provided with extension methods.
in the future, this might prove useful in the way you pointed out. However, I don't think we'd be inclined to interfere with how SE.Redis handles it.
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.
from same object/reference
from single object/reference
- add doc url for configuration
Closes/Fixes #406
Current implementation for setting client lib info on connections is only working for single initial connection in the application context/runtime, which leaves the cases and setups given below out of scope;
There are couple of ways to improve the situation with setting library info on connections but all have its downsides. Here some of them with their own drawbacks;
I choose to follow the third approach since it is clear, reliable and simplistic implementation compared to other two.
In this PR there are three major types introduced, two of them is exposed in public API;
IRedisDatabase : An interface to allow access to modules commands in addition to those in IDatabase
RedisClient : Provides creation+configuration of IRedisDatabase instances
DatabaseWrapper : Underlying wrapper class to compose IDatabase and modules commands into same type.
Existing implementation with extension methods will stay (at least until a major release) and we will keep these new types as experimental, until we make sure this approach brings more benefits, allow required modifications with underlying types and complies with up-coming changes with StackExchange.Redis.