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

Node: Rename instances of Redis to Valkey #2260

Conversation

jonathanl-bq
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@Yury-Fridlyand Yury-Fridlyand left a comment

Choose a reason for hiding this comment

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

Changelog?

node/npm/glide/package.json Outdated Show resolved Hide resolved
node/npm/glide/package.json Show resolved Hide resolved
node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/src/Commands.ts Outdated Show resolved Hide resolved
node/src/GlideClient.ts Outdated Show resolved Hide resolved
node/src/GlideClusterClient.ts Outdated Show resolved Hide resolved
@jonathanl-bq
Copy link
Collaborator Author

Changelog?

You're too fast :P I was in the middle of adding it.

@jonathanl-bq jonathanl-bq marked this pull request as ready for review September 10, 2024 00:19
@jonathanl-bq jonathanl-bq requested a review from a team as a code owner September 10, 2024 00:19
@@ -153,7 +153,7 @@ function initialize() {
RangeByScore,
RangeByLex,
ReadFrom,
RedisCredentials,
ValkeyCredentials,
Copy link
Collaborator

Choose a reason for hiding this comment

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

its server credentials in python and java

Suggested change
ValkeyCredentials,
ServerCredentials,

@@ -265,7 +265,7 @@ function initialize() {
RangeByScore,
RangeByLex,
ReadFrom,
RedisCredentials,
ValkeyCredentials,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
ValkeyCredentials,
ServerCredentials,

@@ -160,7 +160,7 @@ pub fn init(level: Option<Level>, file_name: Option<&str>) -> Level {
logger_level.into()
}

fn redis_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {
fn valkey_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can leave it redis_value_to_js like the other wrappers, since it is a redis value (from redis-rs)

Suggested change
fn valkey_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {
fn redis_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {

@@ -189,7 +189,7 @@ fn redis_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<Js
for (index, item) in array.into_iter().enumerate() {
js_array_view.set_element(
index as u32,
redis_value_to_js(item, js_env, string_decoder)?,
valkey_value_to_js(item, js_env, string_decoder)?,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
valkey_value_to_js(item, js_env, string_decoder)?,
redis_value_to_js(item, js_env, string_decoder)?,

@@ -198,7 +198,7 @@ fn redis_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<Js
let mut obj = js_env.create_object()?;
for (key, value) in map {
let field_name = String::from_owned_redis_value(key).map_err(to_js_error)?;
let value = redis_value_to_js(value, js_env, string_decoder)?;
let value = valkey_value_to_js(value, js_env, string_decoder)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
let value = valkey_value_to_js(value, js_env, string_decoder)?;
let value = redis_value_to_js(value, js_env, string_decoder)?;

@@ -280,7 +280,7 @@ pub fn value_from_split_pointer(
.unwrap();
let pointer = u64::from_le_bytes(bytes);
let value = unsafe { Box::from_raw(pointer as *mut Value) };
redis_value_to_js(*value, js_env, string_decoder)
valkey_value_to_js(*value, js_env, string_decoder)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
valkey_value_to_js(*value, js_env, string_decoder)
redis_value_to_js(*value, js_env, string_decoder)

@@ -411,7 +411,7 @@ class PointerResponse {
}

/** Represents the credentials for connecting to a server. */
export type RedisCredentials = {
export type ValkeyCredentials = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export type ValkeyCredentials = {
export type ServerCredentials = {

@@ -467,7 +467,7 @@ export type BaseClientConfiguration = {
* Credentials for authentication process.
* If none are set, the client will not authenticate itself with the server.
*/
credentials?: RedisCredentials;
credentials?: ValkeyCredentials;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
credentials?: ValkeyCredentials;
credentials?: ServerCredentials;

@@ -20,7 +20,7 @@ import {
RequestError,
Transaction,
} from "..";
import { RedisCluster } from "../../utils/TestUtils.js";
import { ValkeyCluster } from "../../utils/TestUtils.js";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am pretty sure we left it in python as redis cluster as well

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm pretty sure I also have a task to change instances of Redis in Python to Valkey as well

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ohh okay, it doesn't really matter tbh, but it's nice ig

@@ -481,22 +481,22 @@ describe("GlideClient", () => {
);

const result = await client.lolwut();
expect(result).toEqual(expect.stringContaining("Redis ver. "));
expect(result).toEqual(expect.stringContaining("Valkey ver. "));
Copy link
Collaborator

Choose a reason for hiding this comment

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

this will fail

Copy link
Collaborator

Choose a reason for hiding this comment

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

Correct. The output is still "Redis ver. ". Please double check.

@@ -160,7 +160,7 @@ pub fn init(level: Option<Level>, file_name: Option<&str>) -> Level {
logger_level.into()
}

fn redis_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {
fn valkey_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fn valkey_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {
fn resp_value_to_js(val: Value, js_env: Env, string_decoder: bool) -> Result<JsUnknown> {

Signed-off-by: Jonathan Louie <[email protected]>
Copy link
Collaborator

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

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

please double check that you have the latest main changes.

node/src/BaseClient.ts Outdated Show resolved Hide resolved
}
// Negative cursor
if (cluster.checkIfServerVersionLessThan("7.9.0")) {
result = await client.hscan(key1, "-1");
Copy link
Collaborator

Choose a reason for hiding this comment

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

do you want to replace initialCursor throughout?

jonathanl-bq and others added 3 commits September 11, 2024 14:27
Signed-off-by: Jonathan Louie <[email protected]>
Signed-off-by: Jonathan Louie <[email protected]>
@jonathanl-bq jonathanl-bq merged commit ace6c16 into valkey-io:main Sep 11, 2024
15 checks passed
@acarbonetto acarbonetto mentioned this pull request Sep 14, 2024
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants