Skip to content

Clean org#149

Merged
doubleailes merged 4 commits intomainfrom
clean_org
May 3, 2025
Merged

Clean org#149
doubleailes merged 4 commits intomainfrom
clean_org

Conversation

@doubleailes
Copy link
Copy Markdown
Owner

@doubleailes doubleailes commented May 3, 2025

PR Type

Enhancement, Bug fix, Documentation


Description

  • Major: Add new configuration, error handling, payload, and queue modules

    • Implement Config struct with YAML loading, merging, and env var expansion
    • Add comprehensive error types and conversions in GirolleError
    • Introduce Payload struct for argument/keyword serialization and result handling
    • Implement queue/channel creation utilities for AMQP with Lapin
  • Refactor and clean up examples and benches

    • Remove unused imports and simplify code in examples
    • Improve test assertions and code formatting
  • Enhance documentation and code comments

    • Add docstrings and usage examples to new modules and functions
  • Minor: Macro doc formatting and code style improvements


Changes walkthrough 📝

Relevant files
Formatting
3 files
simple_fib.rs
Remove unused serde_json import                                                   
+0/-1     
simple_service.rs
Remove unused serde_json import                                                   
+0/-1     
macro.rs
Clean up imports and formatting in benchmarks                       
+2/-7     
Enhancement
5 files
simple_macro.rs
Simplify return in fibonacci and minor doc fix                     
+1/-1     
config.rs
Add comprehensive configuration struct and YAML/env loading
[link]   
payload.rs
Add Payload struct for argument/result serialization         
[link]   
queue.rs
Add queue/channel creation utilities for AMQP                       
[link]   
types.rs
Add GirolleResult and NamekoFunction type aliases               
[link]   
Error handling
1 files
error.rs
Add detailed error types and conversions for RPC                 
[link]   
Tests
1 files
nameko_utils.rs
Improve test assertion style                                                         
+1/-1     
Documentation
1 files
lib.rs
Improve macro documentation formatting                                     
+1/-2     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @doubleailes doubleailes requested a review from Copilot May 3, 2025 08:30
    Copy link
    Copy Markdown

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR aims to clean up and organize the codebase with minimal formatting and code style improvements.

    • Removed an extra blank line from the macro doc comment.
    • Replaced assert_eq with assert for a more idiomatic approach.
    • Removed unused serde_json imports and streamlined code formatting in benchmark and example files.

    Reviewed Changes

    Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.

    Show a summary per file
    File Description
    girolle_macro/src/lib.rs Removed an extra blank line to tidy up doc comments.
    girolle/src/nameko_utils.rs Replaced assert_eq with assert for improved readability.
    girolle/benches/macro.rs Adjusted import order and reformatted multi-line chaining.
    examples/src/simple_service.rs Removed an unnecessary serde_json import.
    examples/src/simple_macro.rs Removed an explicit return statement, favoring an implicit return.
    examples/src/simple_fib.rs Removed an unnecessary serde_json import.

    @qodo-code-review
    Copy link
    Copy Markdown

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 Security concerns

    Environment variable exposure:
    In the config.rs file, the expand_var function replaces environment variables in configuration strings. This could potentially expose sensitive environment variables if they're included in error messages or logs. Consider adding validation to prevent sensitive environment variables from being used in configurations or ensure they're properly redacted in logs.

    ⚡ Recommended focus areas for review

    Unwrap Usage

    Multiple getter methods in the Config struct use unwrap() on Option values which could lead to panics if the values are None. Consider using safer alternatives like unwrap_or_default() or returning Result types.

    pub fn AMQP_URI(&self) -> String {
        self.AMQP_URI.clone().unwrap()
    }
    /// # prefetch_count
    ///
    /// ## Description
    ///
    /// Function to get the prefetch count.
    ///
    /// ## Returns
    ///
    /// A u16 that holds the prefetch count.
    pub fn prefetch_count(&self) -> u16 {
        self.prefetch_count.unwrap()
    }
    /// # heartbeat
    ///
    /// ## Description
    ///
    /// Function to get the heartbeat.
    ///
    /// ## Returns
    ///
    /// A u16 that holds the heartbeat.
    pub fn heartbeat(&self) -> u16 {
        self.heartbeat.unwrap()
    }
    /// # rpc_exchange
    ///
    /// ## Description
    ///
    /// Function to get the RPC exchange.
    ///
    /// ## Returns
    ///
    /// A String that holds the RPC exchange.
    pub fn rpc_exchange(&self) -> &str {
        self.rpc_exchange.as_ref().unwrap()
    }
    /// # max_workers
    ///
    /// ## Description
    ///
    /// Function to get the max workers.
    ///
    /// ## Returns
    ///
    /// A u32 that holds the max workers.
    pub fn max_workers(&self) -> u32 {
        self.max_workers.unwrap()
    }
    Duplicate FieldTable

    The response_arguments FieldTable is cloned unnecessarily in create_message_channel. Consider restructuring to avoid the clone or add a comment explaining why it's needed.

    response_channel
        .queue_declare(
            rpc_queue_reply,
            QueueDeclareOptions {
                durable: true,
                ..Default::default()
            },
            response_arguments.clone(),
        )
        .await?;
    response_channel
        .basic_qos(prefetch_count, BasicQosOptions::default())
        .await?;
    response_channel
        .queue_bind(
            rpc_queue_reply,
            rpc_exchange,
            &id.to_string(),
            QueueBindOptions::default(),
            response_arguments,
        )
    Error Handling

    The arg() and kwarg() methods use expect() which will panic on serialization failure. Consider returning a Result instead to allow for proper error handling by the caller.

    pub fn arg<T: Serialize>(mut self, arg: T) -> Self {
        self.args
            .push(serde_json::to_value(arg).expect("Failed to serialize argument"));
        self
    }
    /// # kwarg
    ///
    /// ## Description
    ///
    /// push a key value pair to the kwargs HashMap
    ///
    /// ## Example
    ///
    /// ```rust
    /// use girolle::prelude::Payload;
    /// let p = Payload::new().kwarg("key", 1);
    /// ```
    pub fn kwarg<T: Serialize>(mut self, key: &str, value: T) -> Self {
        self.kwargs.insert(
            key.to_string(),
            serde_json::to_value(value).expect("Failed to serialize argument"),
        );
        self
    }

    @qodo-code-review
    Copy link
    Copy Markdown

    qodo-code-review bot commented May 3, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Avoid unnecessary string allocation

    The expand_var() function returns a Cow which is being unnecessarily converted
    to a String and then back to a string slice when passed to
    serde_yaml::from_str(). Remove the .to_string() call to avoid this redundant
    allocation.

    girolle/src/config.rs [116]

    -contents = expand_var(&contents).to_string();
    +contents = expand_var(&contents);
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    __

    Why: The suggestion correctly identifies an unnecessary .to_string() call. Removing it avoids a redundant allocation when passing the Cow<str> returned by expand_var to serde_yaml::from_str, improving efficiency slightly.

    Low
    • Update

    @doubleailes doubleailes merged commit da9d262 into main May 3, 2025
    5 checks passed
    @doubleailes doubleailes deleted the clean_org branch May 3, 2025 08:42
    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.

    2 participants