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

Update dependencies and refactor the FFI calls #375

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

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
src/raw.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@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?

src/context/commands.rs Outdated Show resolved Hide resolved
src/context/mod.rs Outdated Show resolved Hide resolved
src/raw.rs Show resolved Hide resolved
src/raw.rs Show resolved Hide resolved
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
Collaborator

@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.

2 participants