Skip to content

Conversation

@iddm
Copy link
Collaborator

@iddm iddm commented Nov 22, 2023

This greatly simplifies the way the API calls are done, as well as the resulting type conversion is now hidden, which greatly reduces the number of places for the change in the future.

The main change in this PR, besides updating the dependencies, is moving from one way of calling to another, as:

-unsafe { RedisModule_ReplyWithArray.unwrap()(ctx, len).into() }
+redis_call!(RedisModule_ReplyWithArray(ctx, len))

Due to the changed API in the dependencies we use, we had to change the way we call the functions everywhere. By encapsulating the logic inside of a macro like redis_call, the next time we need to do something like this, we only do it in the macro, instead of the whole project.

@iddm iddm requested a review from MeirShpilraien November 22, 2023 14:37
Copy link

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

Few small comments. My main concern is that there is a lot of changes and its hard to spot what we might have missed. This is why I believe this should be part of the next major version. And if we do that, maybe we should already mark the internal functions as pub(crate) so they will not be used from outside wrongly?

Also extracts the only public enum which is used: Status.

Hiding the "raw" module helps to hide the implementation detail and
avoid often API compatibility issues when a change is needed.
Copy link

@MeirShpilraien MeirShpilraien left a comment

Choose a reason for hiding this comment

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

👍
Let make sure this change is only added to the next major version as it contains breaking changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants