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

Rust: modify sorted set command types to RESP3 #912

Merged
merged 1 commit into from
Feb 12, 2024

Conversation

shohamazon
Copy link
Collaborator

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@shohamazon shohamazon requested a review from a team as a code owner February 7, 2024 14:13
@shohamazon shohamazon mentioned this pull request Feb 7, 2024
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
@shohamazon shohamazon force-pushed the zcommands_return_type branch 2 times, most recently from f4fcc2b to 8c491ac Compare February 8, 2024 09:26
@shohamazon shohamazon added the Rust core redis-rs/glide-core matter label Feb 8, 2024
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
@shohamazon shohamazon force-pushed the zcommands_return_type branch 2 times, most recently from dd8a46c to 564c8a1 Compare February 11, 2024 12:45
Copy link
Contributor

@nihohit nihohit left a comment

Choose a reason for hiding this comment

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

please push your rounds as a separate commit instead of squashing.

glide-core/src/client/mod.rs Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
glide-core/src/client/value_conversion.rs Outdated Show resolved Hide resolved
Value::Array(array) => {
let result: Result<Vec<Value>, _> = array
.into_iter()
.map(|element| match element {
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you need the match if you're calling convert_to_expected_type(element, Some(ExpectedReturnType::Double)) internally?

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 am converting string values to double, the rest stays as it is.

Some(ExpectedReturnType::MapOfStringToDouble),
);

match result {
Copy link
Contributor

@nihohit nihohit Feb 11, 2024

Choose a reason for hiding this comment

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

instead of matching on result, just call .unwrap() (in tests)

fix this throughout

.into());
}
let mut inner_iterator = inner_array.into_iter();
let inner_key = inner_iterator.next().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

It's preferable not to unwrap in production code, only in tests. If you're keeping the unwrap because the check above ensures it is safe, add a comment explaining this.

)
.into());
};
map.push((key, value));
Copy link
Contributor

Choose a reason for hiding this comment

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

you're not converting the values, just copying them as-is.

@shohamazon shohamazon force-pushed the zcommands_return_type branch from 564c8a1 to 47f91b0 Compare February 11, 2024 18:25
@shohamazon
Copy link
Collaborator Author

please push your rounds as a separate commit instead of squashing.

Oh sorry I missed that message.

@shohamazon shohamazon force-pushed the zcommands_return_type branch from 47f91b0 to 33863d8 Compare February 11, 2024 21:27
@shohamazon shohamazon requested a review from nihohit February 12, 2024 10:49
.into());
}

array[1] = match convert_to_expected_type(
Copy link
Contributor

Choose a reason for hiding this comment

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

array[1] = convert_to_expected_type( array[1].clone(),
Some(ExpectedReturnType::Double))?;

@shohamazon shohamazon force-pushed the zcommands_return_type branch 2 times, most recently from dbf4dd8 to e8b325d Compare February 12, 2024 15:54
------------

Co-authored-by: Shoham Elias <shohame@amazon.com>
Co-authored-by: Adan Wattad <adanwat@amazon.com>
@shohamazon shohamazon force-pushed the zcommands_return_type branch from e8b325d to 6c18fbd Compare February 12, 2024 15:57
@shohamazon shohamazon merged commit 47bc798 into valkey-io:main Feb 12, 2024
27 checks passed
@shohamazon shohamazon deleted the zcommands_return_type branch February 12, 2024 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Rust core redis-rs/glide-core matter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants