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

feat: add RedisConnection interface and allow database to create conn… #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rishtigupta
Copy link
Owner

@rishtigupta rishtigupta commented Jun 17, 2024

…ection

PR Description:

  • Created a RedisConnection interface to abstract Redis connections
  • Modified implementation to use database for creating connections if Config is not present
  • Updated logic for retrieving target servers to support both Config and database connections

/// <summary>
/// Represents a Redis connection interface.
/// </summary>
public interface IRedisConnection

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where does this get used?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The RedisConnection class implements this interface

@@ -50,7 +50,7 @@ public static IEnumerable<T> Materialize<T>(this IEnumerable<T>? source, bool nu
if (source is null)
{
if (nullToEmpty)
return [];
return new T[0];

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code was not compiling since it gave errors on returning an empty array like so.
So, I google searched and found this solution of returning an empty array

/// <param name="converter">Optional value converter.</param>
/// <param name="handler">Connection event handler.</param>
/// <param name="logger">Logger.</param>
public RedisConnection(IDatabaseAsync database, IValueConverter? converter = null, IConnectionEventHandler? handler = null, TextWriter? logger = null)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah okay so this is the main change in this PR then? Allowing the database to be passed in to the constructor. That makes sense.

In the code samples where you saw CloudStructures being used (both the customer one and the hello world one), I thought I saw some interactions with this class through static methods (meaning that the caller wouldn't necessarily have even called the constructor first). Is that accurate or am I misremembering?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The hello world example, I have been calling the constructor to make a connection while in the customer code, they were directly using the "GetConnection" call from the RedisConnection class. But due to lack of full code, I couldn't determine if they were making a call to the same GetConnection as present in this RedisConnection class.

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

Successfully merging this pull request may close these issues.

2 participants