Skip to content
This repository has been archived by the owner on Jul 6, 2024. It is now read-only.

Minor client updates #213

Merged
merged 10 commits into from
Sep 30, 2022
Merged

Conversation

mjm2312
Copy link
Contributor

@mjm2312 mjm2312 commented Sep 6, 2022

  1. Enhancement: Adds support to update duplicatePolicy, chunkSize in TS.ALTER calls: TS.ALTER

  2. Bug Fix: Changes duplicate policy from boolean | undefined to string | undefined: TS.INFO

  3. Bug Fix: Allows a list of args for TS.QUERYINDEX TS.QUERYINDEX

@danitseitlin
Copy link
Owner

@mjm2312 Hi, please notice to the CI failures, once all is passing I will review :)
Thanks for the PR 🔥

@danitseitlin
Copy link
Owner

@mjm2312 ping?

@mjm2312
Copy link
Contributor Author

mjm2312 commented Sep 26, 2022

@mjm2312 ping?

hey Dani, sorry for the delay. It seems like relevant tests are passing now.

Copy link
Owner

@danitseitlin danitseitlin left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, added a few comments :)

modules/rts/rts.commander.ts Outdated Show resolved Hide resolved
modules/rts/rts.ts Outdated Show resolved Hide resolved
modules/rts/rts.types.ts Outdated Show resolved Hide resolved
@danitseitlin
Copy link
Owner

Another thing, please make sure to point out you're updating the RTS client in the final merged commit :)

@mjm2312
Copy link
Contributor Author

mjm2312 commented Sep 26, 2022

Another thing, please make sure to point out you're updating the RTS client in the final merged commit :)

I can squash and merge, and include in the message that it's for RTS and addresses the three points at the top of the PR

Copy link
Owner

@danitseitlin danitseitlin left a comment

Choose a reason for hiding this comment

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

A few comments, in addition notice some tests failed on the build.
I think it's some tests that broke after the change from String to the custom type in Duplicate Policy. Probably needs a small fix.

modules/rts/rts.types.ts Outdated Show resolved Hide resolved
modules/rts/rts.commander.ts Outdated Show resolved Hide resolved
modules/rts/rts.commander.ts Outdated Show resolved Hide resolved
Copy link
Owner

@danitseitlin danitseitlin left a comment

Choose a reason for hiding this comment

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

Great job! Thanks

@mjm2312
Copy link
Contributor Author

mjm2312 commented Sep 27, 2022

Great job! Thanks

np! I don't have write access so not sure I can merge

Could you either grant me write access or merge yourself? Thank you :)

@danitseitlin danitseitlin merged commit 42b4ceb into danitseitlin:master Sep 30, 2022
danitseitlin pushed a commit that referenced this pull request Mar 23, 2023
* Enhancement: Adds support to update duplicatePolicy, chunkSize in TS.ALTER calls: [TS.ALTER](https://redis.io/commands/ts.alter/)
* Bug Fix: Changes duplicate policy from boolean | undefined to string | undefined: [TS.INFO](https://redis.io/commands/ts.alter/)
* Bug Fix: Allows a list of args for TS.QUERYINDEX [TS.QUERYINDEX](https://redis.io/commands/ts.queryindex/)
danitseitlin pushed a commit that referenced this pull request Mar 23, 2023
* Enhancement: Adds support to update duplicatePolicy, chunkSize in TS.ALTER calls: [TS.ALTER](https://redis.io/commands/ts.alter/)
* Bug Fix: Changes duplicate policy from boolean | undefined to string | undefined: [TS.INFO](https://redis.io/commands/ts.alter/)
* Bug Fix: Allows a list of args for TS.QUERYINDEX [TS.QUERYINDEX](https://redis.io/commands/ts.queryindex/)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants