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

Making command structs digestable #2716

Merged
merged 5 commits into from
Sep 20, 2023
Merged

Making command structs digestable #2716

merged 5 commits into from
Sep 20, 2023

Conversation

chayim
Copy link
Contributor

@chayim chayim commented Sep 19, 2023

The Cmdable interface (and files) are far too large to digest. This is a first step towards making these pieces easier to modify. Commands have been grouped like other clients and the command reference.

I think a reasonable second step would be to separate these interfaces into an internal package, and add a layer of abstraction. But this is for contributor ergonomics, not necessarily user facing value.

Cmdable has been split up into a series of files whose suffix is command and a new Cmdable interface added per group - extending Cmdable. For example ACL commands have been moved to acl_commands.go and AclCmdable added to the Cmdable interface.

@chayim chayim marked this pull request as ready for review September 19, 2023 15:09
Copy link
Contributor

@SoulPancake SoulPancake left a comment

Choose a reason for hiding this comment

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

This is neat.
But I've noticed that this approach is leading to a proliferation of files directly in the root of the redis package.
Don't you think as the project grows, having a cluttered root directory can make it harder to manage and make modifications?
Can we eventually just have sub-directories for these perhaps
@chayim

@SoulPancake
Copy link
Contributor

And in that case, do you think we should also have the test files in this manner for each of the family of command structs? @chayim

@chayim
Copy link
Contributor Author

chayim commented Sep 20, 2023

100% agree on sub directories. I FULLY FULLY agree that longer term we should pull everything out into a sub package. That's why this is phase one.

I didn't move tests around for this reason.. As we should also move the various parsers (that's what started this). IMHO for a first phase - and maybe someone (@ofekshenawa @SoulPancake ) picking it up... I hope to provide direction.

I don't want to do a massive set of changes here as a single PR. Surgical changes! Fair?

@ofekshenawa ofekshenawa merged commit 6199a2a into master Sep 20, 2023
9 checks passed
ClusterCmdable
ScriptingFunctionsCmdable
StringCmdable
PubSubCmdable
GearsCmdable
ProbabilisticCmdable
TimeseriesCmdable
Copy link

Choose a reason for hiding this comment

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

Did you guys intend to omit StreamCmdable from this list?

Choose a reason for hiding this comment

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

This is a significant breaking change without StreamCmdable available

Choose a reason for hiding this comment

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

@chayim Can you verify if this was intentional? If not, I think we need an urgent new point release including it. If it was intentional, it should be added to the release notes with workaround / migration instructions. Thanks in advance!

Choose a reason for hiding this comment

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

looks like there's a PR in flight to fix this #2725

Copy link
Contributor

Choose a reason for hiding this comment

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

BitMapCmdable is also missing.

PR to fix -> #2737

@ofekshenawa ofekshenawa deleted the ck-movify branch June 19, 2024 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants