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(cli): use editor for workon and chat on rs CLI #63

Merged
merged 5 commits into from
Nov 24, 2024
Merged

feat(cli): use editor for workon and chat on rs CLI #63

merged 5 commits into from
Nov 24, 2024

Conversation

skyl
Copy link
Owner

@skyl skyl commented Nov 24, 2024

PR Type

enhancement


Description

  • Replaced dialoguer::Input with get_user_input_via_editor for multiline input in chat and workon commands.
  • Added new functions in context module for handling editor-based input and styled message printing.
  • Updated docker-compose.yaml to include a commented-out EDITOR environment variable.

Changes walkthrough 📝

Relevant files
Enhancement
chat.rs
Switch to editor-based input for chat command                       

rs/core/corpora_cli/src/commands/chat.rs

  • Removed the use of dialoguer::Input for user input.
  • Introduced get_user_input_via_editor for multiline input.
  • +9/-8     
    workon.rs
    Use editor for input in workon command                                     

    rs/core/corpora_cli/src/commands/workon.rs

  • Removed dialoguer::Input for user input.
  • Added get_user_input_via_editor for multiline input.
  • Simplified file path handling and printing.
  • +18/-45 
    mod.rs
    Add editor-based input handling in context                             

    rs/core/corpora_cli/src/context/mod.rs

  • Added functions for printing styled messages.
  • Implemented get_user_input_via_editor function.
  • Introduced temporary file handling for editor input.
  • +72/-1   
    Configuration changes
    docker-compose.yaml
    Update docker-compose with editor variable comment             

    docker-compose.yaml

    • Commented out EDITOR environment variable.
    +1/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    The expect("FML") call in get_user_input_via_editor could be improved with a more descriptive error message to aid in debugging.

    Error Handling
    The expect("Failed to get user input") call in get_user_input_via_editor could be improved with a more descriptive error message to aid in debugging.

    Editor Command Handling
    The logic for adding flags to non-blocking editors could be more robust by checking for exact matches or using a more reliable method to determine the editor type.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Ensure a fallback mechanism is in place if the default editor is unavailable

    Handle the case where the EDITOR environment variable is not set and the default
    editor (vim) is not available on the system, to prevent potential failures when
    attempting to open the editor.

    rs/core/corpora_cli/src/context/mod.rs [199]

    -let editor = std::env::var("EDITOR").unwrap_or_else(|_| "vim".to_string());
    +let editor = std::env::var("EDITOR").unwrap_or_else(|_| {
    +    if which::which("vim").is_ok() {
    +        "vim".to_string()
    +    } else {
    +        panic!("No editor found. Please set the EDITOR environment variable.")
    +    }
    +});
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a critical issue where the absence of the default editor 'vim' could lead to a failure. By checking for the availability of 'vim' and providing a clear error message if no editor is found, it significantly enhances the robustness of the code.

    9
    General
    Improve error handling for path prefix stripping to prevent unexpected panics

    Add error handling for the strip_prefix method to provide a more descriptive error
    message or handle the error gracefully if the path cannot be stripped.

    rs/core/corpora_cli/src/context/mod.rs [22-23]

     let relative_path = absolute_path
         .strip_prefix(&ctx.corpora_config.root_path)
    -    .expect("Failed to get relative path");
    +    .unwrap_or_else(|_| panic!("Failed to strip prefix from path: {}", absolute_path.display()));
    Suggestion importance[1-10]: 7

    Why: The suggestion improves error handling by providing a more descriptive error message when the strip_prefix operation fails. This enhances the code's robustness and helps in debugging by clarifying the cause of the error.

    7

    @skyl skyl merged commit 9db7571 into main Nov 24, 2024
    1 check passed
    @skyl skyl deleted the editor branch November 24, 2024 02:28
    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.

    1 participant