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

Core: Add response conversion for FUNCTION LIST command #1489

Merged

Conversation

Yury-Fridlyand
Copy link
Collaborator

Issue #, if available:
N/A

Description of changes:
As requested in #1452 (review) moving rust part into a separate PR

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

@Yury-Fridlyand Yury-Fridlyand added the Rust core redis-rs/glide-core matter label May 29, 2024
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand force-pushed the core/integ_yuryf_flist_conversion branch from a0ba60e to fa3c80a Compare May 29, 2024 15:25
@Yury-Fridlyand Yury-Fridlyand marked this pull request as ready for review May 29, 2024 15:25
@Yury-Fridlyand Yury-Fridlyand requested a review from a team as a code owner May 29, 2024 15:25
@Yury-Fridlyand Yury-Fridlyand requested a review from barshaul May 29, 2024 15:25
@@ -22,6 +22,8 @@ pub(crate) enum ExpectedReturnType {
Lolwut,
ArrayOfStringAndArrays,
ArrayOfArraysOfDoubleOrNull,
ArrayOfMaps(&'a ExpectedReturnType<'a>),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rustc required to use a reference, otherwise enum size is unlimited and then it required to define a lifetime specifier ('a)

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@@ -5,8 +5,8 @@ use redis::{
cluster_routing::Routable, from_owned_redis_value, Cmd, ErrorKind, RedisResult, Value,
};

#[derive(Clone, Copy)]
pub(crate) enum ExpectedReturnType {
#[derive(Clone, Copy, Debug)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure we should add this to release code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/aws/glide-for-redis/pull/1487/files#diff-eaea3b88c569ecab22aeb7db41a9dc796aedb808b65d7f3ceceacc551b8a35c0R43
It generates code for debug string formatting ({:?}) we use for error messages. And I think if we don't use it, the code is dropped by optimizer.

…ist_conversion

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand
Copy link
Collaborator Author

@barshaul round

…ist_conversion

Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
@Yury-Fridlyand Yury-Fridlyand merged commit f41a13c into valkey-io:main Jun 5, 2024
46 checks passed
@Yury-Fridlyand Yury-Fridlyand deleted the core/integ_yuryf_flist_conversion branch June 5, 2024 01:50
yipin-chen pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 7, 2024
…1489)


Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024
…1489)


Signed-off-by: Yury-Fridlyand <yury.fridlyand@improving.com>
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