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

feat: redis configuration url #1577

Merged
merged 1 commit into from
Jul 31, 2024
Merged

feat: redis configuration url #1577

merged 1 commit into from
Jul 31, 2024

Conversation

dinhani-cw
Copy link
Contributor

@dinhani-cw dinhani-cw commented Jul 31, 2024

PR Type

Enhancement, Error handling


Description

  • Enhanced PermanentStorageConfig to require perm_storage_url when perm_storage_kind is set to redis.
  • Improved error handling for missing Redis URL in PermanentStorageConfig.
  • Modified RedisPermanentStorage::new to accept a URL parameter.
  • Added error handling for Redis client creation failure in RedisPermanentStorage.

Changes walkthrough 📝

Relevant files
Enhancement
permanent_storage.rs
Require and validate Redis URL in `PermanentStorageConfig`

src/eth/storage/permanent_storage.rs

  • Added log_and_err import.
  • Updated PermanentStorageConfig to require perm_storage_url if
    perm_storage_kind is redis.
  • Enhanced error handling for missing Redis URL.
  • +8/-2     
    Error handling
    redis_permanent.rs
    Accept URL parameter and handle errors in Redis client creation

    src/eth/storage/redis/redis_permanent.rs

  • Modified RedisPermanentStorage::new to accept a URL parameter.
  • Added error handling for Redis client creation failure.
  • +5/-2     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Error Handling
    The error message for missing Redis URL should include more context, such as the expected configuration. Consider enhancing the message to guide the user on how to resolve the issue.

    Error Logging
    The error handling for Redis client creation should log the original error in a field called reason. This will help in debugging by providing more context about the failure.

    Copy link

    github-actions bot commented Jul 31, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use anyhow::bail! for error handling instead of a custom macro

    Instead of using log_and_err! macro directly in the match arm, consider using
    anyhow::bail! for better error handling and consistency with the anyhow crate.

    src/eth/storage/permanent_storage.rs [118]

    -return log_and_err!("redis connection url not provided when it was expected to be present");
    +anyhow::bail!("redis connection url not provided when it was expected to be present");
     
    Suggestion importance[1-10]: 8

    Why: Using anyhow::bail! aligns with the anyhow crate's error handling practices, improving consistency and readability. This is a best practice suggestion that enhances maintainability.

    8
    Use anyhow::Context to provide context to errors instead of a custom macro

    Instead of using log_and_err! macro for logging and returning an error, consider
    using anyhow::Context to provide context to the error, which integrates well with
    the anyhow crate.

    src/eth/storage/redis/redis_permanent.rs [39]

    -Err(e) => return log_and_err!(reason = e, "failed to create redis client"),
    +Err(e) => return Err(e).context("failed to create redis client"),
     
    Suggestion importance[1-10]: 8

    Why: Utilizing anyhow::Context integrates well with the anyhow crate and provides better error context, which is a best practice for error handling in Rust.

    8
    Enhancement
    Add a debug log statement before returning an error for missing Redis connection URL

    Add a debug! log statement before returning the error to provide more context in the
    logs when the Redis connection URL is not provided.

    src/eth/storage/permanent_storage.rs [118]

    +debug!("Redis connection URL was expected but not provided");
     return log_and_err!("redis connection url not provided when it was expected to be present");
     
    Suggestion importance[1-10]: 7

    Why: Adding a debug log statement can provide more context in logs, which is useful for debugging. This is a minor enhancement that improves logging.

    7
    Readability
    Use if let syntax for better readability when checking for the Redis connection URL

    Consider using if let syntax for better readability when checking if the Redis
    connection URL is provided.

    src/eth/storage/permanent_storage.rs [117-120]

    -let Some(url) = self.perm_storage_url.as_deref() else {
    +if let Some(url) = self.perm_storage_url.as_deref() {
    +    Box::new(RedisPermanentStorage::new(url)?)
    +} else {
         return log_and_err!("redis connection url not provided when it was expected to be present");
    -};
    +}
     
    Suggestion importance[1-10]: 6

    Why: The if let syntax improves readability and is more idiomatic in Rust. This suggestion enhances code readability and maintainability.

    6

    @dinhani-cw dinhani-cw enabled auto-merge (squash) July 31, 2024 00:11
    @dinhani-cw dinhani-cw merged commit 9a68e1b into main Jul 31, 2024
    34 checks passed
    @dinhani-cw dinhani-cw deleted the redis-config branch July 31, 2024 00:15
    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.

    1 participant