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.

Sign up for GitHub

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

Jump to bottom

Implement FromRedisValue for Box<[T]> and Arc<[T]> #799

Merged
merged 8 commits into from
Nov 4, 2023
Merged

Implement FromRedisValue for Box<[T]> and Arc<[T]> #799

merged 8 commits into from
Nov 4, 2023

Conversation

JOT85
Copy link
Contributor

@JOT85 JOT85 commented Mar 9, 2023

No description provided.

@nihohit
Copy link
Contributor

nihohit commented Mar 22, 2023

please add tests.

nihohit
nihohit reviewed Mar 23, 2023
View reviewed changes
redis/src/types.rs Outdated Show resolved Hide resolved
nihohit
nihohit approved these changes Mar 23, 2023
View reviewed changes
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.

LGTM

@nihohit nihohit mentioned this pull request Sep 24, 2023
Closed
@JOT85
Copy link
Contributor Author

JOT85 commented Sep 26, 2023

@nihohit would you like me to make those changes to this branch? It looks like #961 might have superseded this?

@nihohit
Copy link
Contributor

nihohit commented Sep 26, 2023

@JOT85 sorry, I wasn't sure if you're still interested in this, so I moved forward and integrated your change in #961.
If you still want, you're more than welcome to update your PR either with my changes, or your version - you created this first, and if you want this contribution under your name, it's rightly yours.

@JOT85
Copy link
Contributor Author

JOT85 commented Oct 10, 2023

Sorry it took so long to get round to making that change!

I switched the impls to a macro, and also added an impl for Arc<[T]>.

The differences between the implementation I used are:

  • The macro generates the entire impl, not just the function.
  • The error message includes the collection type, not just the inner type, for example it's Conversion to Box<[u32]> failed instead of Conversion to u32 failed.

@JOT85 JOT85 changed the title Implement FromRedisValue for Box<[T]> Implement FromRedisValue for Box<[T]> and Arc<[T]> Oct 10, 2023
nihohit
nihohit approved these changes Nov 3, 2023
View reviewed changes
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.

@jaymell LGTM.
@JOT85 The test code has quite a bit of repetition, it would be nice if some of that can be reduced.

@jaymell jaymell merged commit cdf3a94 into redis-rs:main Nov 4, 2023
7 of 10 checks passed
@jaymell
Copy link
Contributor

jaymell commented Nov 4, 2023

@jaymell LGTM. @JOT85 The test code has quite a bit of repetition, it would be nice if some of that can be reduced.

Thanks! -- I'll go ahead and merge since we've been sitting on this awhile.

barshaul pushed a commit to amazon-contributing/redis-rs that referenced this pull request Dec 7, 2023
@nihohit @JOT85
@sp-angel @eythorhel19 @thorhelgason @ruanpetterson @jaymell @aumetra @joshleeb @Ayush1325
Merge updates from upstream. (#67)
eb8d19d 
* Disable JSON module tests for redis 6.2.4. (redis-rs#980)

This happens due to a consistent crash -
RedisJSON/RedisJSON#1124

* Implement `FromRedisValue` for `Box<[T]>` and `Arc<[T]>` (redis-rs#799)

* Update minimal rust version to 1.6.5 (redis-rs#982)

It's required by regex v1.10.2.
https://github.com/redis-rs/redis-rs/actions/runs/6760188489/job/18373652560?pr=981

* Support Mutual TLS (redis-rs#858)

Add support for mutual TLS. Enable specifying client
certificate and key and (optional) root certificate.

* changing timeouts from usize and isize to f64 (redis-rs#988)

Co-authored-by: Eythor Helgason <eytorhelgason@gmail.com>

* Accept iterator at `ClusterClient` initialization (redis-rs#987)

* Prefer routing to primary in a transaction. (redis-rs#986)

* Rename route_pipeline to route_for_pipeline.

There are 2 `route_pipeline` functions in the file, so we change one to
avoid confusion.

* Prefer routing to primary in a transaction.

* CrossSlot error on pipeline with many routes.

* Sync Pub/Sub - cache received pub/sub messages. (redis-rs#910)

When sending subscibre/unsubscribe messages to the server, the pub/sub
client might receive messages that it can mistake to be the response,
and not return to the next `get_message` call. This fix ensures that
pub/sub messages will be queued until the next `get_message` call, so
that no message will be missed.

* Fix sync cluster behavior with transactions. (redis-rs#983)

The leading MULTI prevents the transaction from being properly routed.
In order to route the request correctly, we drop the leading MULTI from
the routable.

* Release redis 0.23.4 / redis-test 0.2.4 (redis-rs#992)

* Tests: Add retries to test cluster creation (redis-rs#994)

* Revert redis-test versioning changes (redis-rs#993)

Publishing to crates.io without the version specification
is currently not possible. Reverting these changes for now
until we have a better solution/workaround.

* Fix features for `load_native_certs`. (redis-rs#996)

The directive now matches the directives wrapping `load_native_certs`'s
`use` statements. This would fail compilation for anyone accidentally
using the "tls" feature with rustls.

* Bump aHash to v0.8.6 (redis-rs#966)

* Update Command expiration values to be an appropriate type (redis-rs#589)

This change updates expiration values provided to some commands in the
`Command` trait to be of type `i64` or `u64` where appropriate,
replacing the previously used `usize` type.

More specifically, the commands that are updated are

- SETEX, PSETEX
  - Expiration value changed from `usize` to `u64`
- EXPIRE, PEXPIRE, EXPIREAT, PEXPIREAT
  - Expiration value changed from `usize` to `i64`

For the `*SETEX` commands, there is no mention of supporting negative
values. And indeed, providing a negative value through redis-cli returns
an error message.

For the `*EXPIRE*` commands, I couldn't find the paragraph in the docs
referenced in redis-rs#575, nor any other mention that the commands accept a
negative expiration argument. However, in testing with redis-cli it
seems a negative expiration value is valid and in checking the code I
found it to confirm these observations. See [here][code1] and
[here][code2].

Note that this is a breaking change.

[code1]: https://github.com/redis/redis/blob/e3ef73dc2a557232c60c732705e8e6ff2050eba9/src/expire.c#L570-L571
[code2]: https://github.com/redis/redis/blob/e3ef73dc2a557232c60c732705e8e6ff2050eba9/src/expire.c#L483-L491

* Fix StreamId::contains_key signature (redis-rs#783)

* Order in usage of ClusterParams. (redis-rs#997)

* Move `tls_params` into `ClusterParams`.

The two objects are used together, so it makes sense to join them.

* Cluster: Contain `ClusterParams` internally.

This matches the async cluster's structure, and ensures that fields that
are added to `ClusterParams` are automatically added to the sync cluster

* Release redis 0.24.0 / redis-test 0.3.0 (redis-rs#998)

---------

Co-authored-by: Jacob O'Toole <jacob@jotpot.co.uk>
Co-authored-by: spaceangel <129979014+sp-angel@users.noreply.github.com>
Co-authored-by: eythorhel19 <70693645+eythorhel19@users.noreply.github.com>
Co-authored-by: Eythor Helgason <eytorhelgason@gmail.com>
Co-authored-by: Ruan Petterson <ruan@petterson.eng.br>
Co-authored-by: James Lucas <jaymell@users.noreply.github.com>
Co-authored-by: aumetra <aumetra@cryptolab.net>
Co-authored-by: Josh Leeb-du Toit <mail@joshleeb.com>
Co-authored-by: Ayush <ayushsingh1325@gmail.com>
barshaul pushed a commit to barshaul/redis-rs that referenced this pull request Jul 11, 2024
@shachlanAmazon @nihohit
Update CD in order to publish packages. (redis-rs#799)
d5345a7 
* Update CD with new arguments.

* Update NPM package name to account for AWS org.

---------

Co-authored-by: nihohit <nihohit@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaymell jaymell jaymell left review comments

@nihohit nihohit nihohit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@JOT85 @nihohit @jaymell