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

Code cleanup: removing unused dependencies in tests, removing digest file as it can be taken from a dependency and moving metric increments to after creates #44

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

zackcam
Copy link
Contributor

@zackcam zackcam commented Jan 31, 2025

This pr removes unused dependencies from python integration tests and consolidates dependencies onto one line if possible

Removed the digest.rs file as the file was designed to be temporary until valkey-modulers released the digest changes which it has. Due to this updated the import of the digest to now be from valkey_module

Updated two order of incrementing metrics, in copy we now only increment the metrics after the create has gone through which means any unexpected failures on the create now won't cause issues with metrics. For decode moved the increment above the error check after create this now means if it gets dropped from the size validation the metrics will be accurate.

Had one clippy fix as well shown below:

            let new_fp_rate =
                match Self::calculate_fp_rate(self.fp_rate, num_filters, self.tightening_ratio) {
                    Ok(rate) => rate,
                    Err(e) => return Err(e),
                };
                Self::calculate_fp_rate(self.fp_rate, num_filters, self.tightening_ratio)?;

Updated the command syntax in lib.rs to now follow the new args added for INFO and INSERT commands

file as it can be taken from a dependency and moving metric increments
to after creates

Signed-off-by: zackcam <[email protected]>
@KarthikSubbarao KarthikSubbarao merged commit 8077d58 into valkey-io:unstable Feb 3, 2025
6 checks passed
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