Skip to content

Conversation

@niechen
Copy link
Contributor

@niechen niechen commented Jul 10, 2025

User description

Summary

  • Changed argument parsing in mcpm edit command from comma-separated to space-separated format
  • Updated all prompts and display formats to reflect this change
  • Modified tests to match new behavior

Context

This change improves the user experience by making argument input more natural. Users can now enter arguments separated by spaces instead of commas.

Before

Arguments (comma-separated): arg1,arg2,arg3

After

Arguments (space-separated): arg1 arg2 arg3

Changes Made

  • Updated argument parsing from .split(",") to .split() in all relevant locations
  • Changed prompt messages from "comma-separated" to "space-separated"
  • Updated table display format to show arguments with spaces
  • Modified test expectations to match new format

Test Plan

  • All existing tests pass
  • Manual testing of mcpm edit command works as expected
  • Code passes ruff linting and formatting checks

Fixes #212

🤖 Generated with Claude Code


PR Type

Bug fix


Description

  • Changed argument parsing from comma-separated to space-separated format

  • Updated prompts and display messages to reflect new format

  • Modified tests to expect space-separated output


Changes diagram

flowchart LR
  A["CSV Format"] -- "Replace .split(',')" --> B["Space Format"]
  B -- "Update prompts" --> C["User Interface"]
  B -- "Update display" --> D["Table Output"]
  B -- "Update tests" --> E["Test Expectations"]
Loading

Changes walkthrough 📝

Relevant files
Bug fix
edit.py
Convert CSV argument parsing to space-separated                   

src/mcpm/commands/edit.py

  • Changed argument parsing from .split(",") to .split()
  • Updated prompts from "comma-separated" to "space-separated"
  • Modified table display to show space-separated arguments
  • Simplified argument parsing logic in multiple functions
  • +9/-11   
    Tests
    test_edit.py
    Update test to expect space-separated output                         

    tests/test_edit.py

    • Updated test expectation from "arg1, arg2" to "arg1 arg2"
    +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • - Updated argument parsing from comma-separated to space-separated
    - Changed prompts from "Arguments (comma-separated)" to "Arguments (space-separated)"
    - Modified parsing logic from .split(",") to .split()
    - Updated table display format to show space-separated arguments
    - Updated tests to expect space-separated output
    
    Fixes #212
    
    🤖 Generated with [Claude Code](https://claude.ai/code)
    
    Co-Authored-By: Claude <[email protected]>
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    212 - Fully compliant

    Compliant requirements:

    • Change mcpm edit command to use space-separated arguments instead of CSV format
    • Make argument input more natural for users
    • Update argument parsing from comma-separated to space-separated

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

    Argument Handling

    The change from comma-separated to space-separated arguments may break existing workflows where users have arguments containing spaces. The new parsing logic using split() without parameters will split on any whitespace, potentially breaking arguments that contain spaces.

    new_args = answers["args"].split() if answers["args"] else []
    console.print(f"Arguments: [cyan]{server_config.args}[/] → [cyan]{new_args}[/]")

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jul 10, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle arguments with spaces properly

    The current implementation doesn't handle arguments with spaces correctly.
    Arguments containing spaces will be incorrectly split into multiple arguments,
    breaking functionality for commands that need quoted arguments.

    src/mcpm/commands/edit.py [240]

    -new_args = answers["args"].split() if answers["args"] else []
    +import shlex
    +new_args = shlex.split(answers["args"]) if answers["args"] else []

    [Suggestion processed]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that using split() is insufficient for parsing command-line arguments that may contain spaces within quotes, and proposes using shlex.split() which is the standard and correct approach for this task.

    Medium
    Use proper argument parsing

    Using simple split() will break arguments that contain spaces. This could cause
    command execution failures when arguments need to be quoted or contain
    whitespace.

    src/mcpm/commands/edit.py [301]

    -server_config.args = answers["args"].split()
    +import shlex
    +server_config.args = shlex.split(answers["args"])
    • Apply / Chat
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that using split() is insufficient for parsing command-line arguments that may contain spaces within quotes, and proposes using shlex.split() which is the standard and correct approach for this task.

    Medium
    Fix argument parsing for spaces

    Simple string splitting will incorrectly handle arguments containing spaces,
    potentially breaking server configurations that require quoted arguments or
    paths with spaces.

    src/mcpm/commands/edit.py [384]

    -args=result["answers"]["args"].split() if result["answers"]["args"] else [],
    +import shlex
    +args=shlex.split(result["answers"]["args"]) if result["answers"]["args"] else [],

    [To ensure code accuracy, apply this suggestion manually]

    Suggestion importance[1-10]: 8

    __

    Why: The suggestion correctly identifies that using split() is insufficient for parsing command-line arguments that may contain spaces within quotes, and proposes using shlex.split() which is the standard and correct approach for this task.

    Medium
    • Update

    @github-actions
    Copy link
    Contributor

    Summary

    Switches STDIOServerConfig argument handling from comma-separated to space-separated strings, updating the edit command logic, prompts, and related tests.

    Review

    Nice quality-of-life tweak—code, prompts, and tests stay in sync and the diff is clean.

    • ✅ Logic updated consistently (edit.py, new-server flow, apply changes).
    • ✅ UX wording adapted to “space-separated”.
    • ✅ Tests adjusted to the new output.
      Looks good to merge!

    View workflow run

    - Import shlex module for proper shell-like argument parsing
    - Replace .split() with shlex.split() for argument parsing
    - Update prompts to indicate quote support for arguments with spaces
    - Add tests for arguments containing spaces
    - Add unit tests to verify shlex parsing behavior
    
    This addresses the review feedback about handling arguments with spaces correctly.
    Arguments like "path with spaces" are now properly parsed as single arguments.
    
    🤖 Generated with [Claude Code](https://claude.ai/code)
    
    Co-Authored-By: Claude <[email protected]>
    @niechen
    Copy link
    Contributor Author

    niechen commented Jul 10, 2025

    🔧 Review Feedback Addressed

    Thank you for the thorough review! I've addressed the concerns about handling arguments with spaces:

    Changes Made:

    1. Proper argument parsing: Replaced simple .split() with shlex.split() for shell-like parsing
    2. Quoted argument support: Arguments with spaces can now be properly quoted
    3. Updated prompts: Changed to "Arguments (space-separated, quotes supported)" with helpful instructions
    4. Added tests: New test cases verify that arguments with spaces work correctly

    Examples of what now works:

    • Basic: arg1 arg2 arg3
    • With spaces: arg1 "arg with spaces" arg3
    • File paths: "path/to/file with spaces.txt" --flag
    • Mixed quotes: arg1 'single quoted' "double quoted"

    Testing:

    • ✅ All existing tests pass (113 passed, 6 skipped)
    • ✅ Added specific test for arguments with spaces
    • ✅ Added unit tests for shlex parsing behavior
    • ✅ Code passes ruff linting and formatting

    The implementation now correctly handles the edge cases mentioned in the review while maintaining backward compatibility for simple space-separated arguments.

    @niechen niechen merged commit 85a3492 into main Jul 10, 2025
    7 checks passed
    @niechen niechen deleted the feat/issue-212-space-separated-args branch July 10, 2025 10:52
    mcpm-semantic-release bot pushed a commit that referenced this pull request Jul 10, 2025
    # [2.4.0](v2.3.0...v2.4.0) (2025-07-10)
    
    ### Features
    
    * change mcpm edit arguments from CSV to space-separated format ([#213](#213)) ([85a3492](85a3492)), closes [#212](#212)
    @mcpm-semantic-release
    Copy link

    🎉 This PR is included in version 2.4.0 🎉

    The release is available on GitHub release

    Your semantic-release bot 📦🚀

    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.

    [Bug]: stdio arguments should not use csv

    2 participants