Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented May 20, 2025

PR Type

Bug fix


Description

  • Exit CLI if formatter not installed

  • Detect Black or uv formatter commands

  • Import subprocess, sys, and click modules

  • Gracefully handle missing executable errors


Changes walkthrough 📝

Relevant files
Error handling
main.py
Verify formatter installation at runtime                                 

codeflash/main.py

• Added subprocess, sys, and click imports
• Checks if formatter
command is Black or uv
• Runs formatter to verify installation

Catches missing executable and exits

+12/-1   

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @github-actions
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    IndexError risk

    Directly accessing args.formatter_cmds[0] without validating that the list is non-empty can raise an IndexError if no formatter commands are provided.

    if args.formatter_cmds[0].startswith("black") or args.formatter_cmds[0].startswith("uv"):
        formatter = args.formatter_cmds[0].split(" ")[0]
        try:
            subprocess.run([formatter], capture_output=True, check=False)
        except (FileNotFoundError, NotADirectoryError):
            click.echo(f"⚠️ Formatter not found: {formatter}, please ensure it is installed")
            sys.exit(1)
    Formatter existence check

    Calling subprocess.run([formatter]) without evaluating its exit code or using shutil.which may not reliably detect whether the executable is present.

    if args.formatter_cmds[0].startswith("black") or args.formatter_cmds[0].startswith("uv"):
        formatter = args.formatter_cmds[0].split(" ")[0]
        try:
            subprocess.run([formatter], capture_output=True, check=False)
        except (FileNotFoundError, NotADirectoryError):
            click.echo(f"⚠️ Formatter not found: {formatter}, please ensure it is installed")
            sys.exit(1)

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Check list is non-empty first

    Guard against an empty formatter_cmds list to prevent an IndexError by ensuring it
    has at least one element before accessing index 0.

    codeflash/main.py [42]

    -if args.formatter_cmds[0].startswith("black") or args.formatter_cmds[0].startswith("uv"):
    +if args.formatter_cmds and (args.formatter_cmds[0].startswith("black") or args.formatter_cmds[0].startswith("uv")):
    Suggestion importance[1-10]: 7

    __

    Why: Guarding against an empty formatter_cmds list avoids a potential IndexError, improving robustness without much overhead.

    Medium
    General
    Add version flag to formatter check

    Use a harmless flag like --version when invoking the formatter to avoid hangs or
    unexpected errors when no file argument is provided. This verifies installation
    without requiring actual formatting.

    codeflash/main.py [45]

    -subprocess.run([formatter], capture_output=True, check=False)
    +subprocess.run([formatter, "--version"], capture_output=True, check=False)
    Suggestion importance[1-10]: 6

    __

    Why: Adding --version prevents the formatter from hanging or attempting to read STDIN without arguments, improving the reliability of the check.

    Low
    Use shlex for command parsing

    Use shlex.split to correctly parse the command string, handling quoted arguments or
    extra spaces more robustly.

    codeflash/main.py [43]

    -formatter = args.formatter_cmds[0].split(" ")[0]
    +import shlex
    +formatter = shlex.split(args.formatter_cmds[0])[0]
    Suggestion importance[1-10]: 5

    __

    Why: Using shlex.split more accurately parses command strings, though the impact is minor and the import placement may need adjustment.

    Low

    @aseembits93 aseembits93 changed the title Exit the cli if user doesn't have the formatter installed while running codeflash Exit the cli if user doesn't have the formatter installed while running codeflash (CF-644) May 20, 2025
    @aseembits93 aseembits93 requested a review from misrasaurabh1 May 20, 2025 18:22
    @aseembits93 aseembits93 enabled auto-merge May 20, 2025 18:50
    @aseembits93
    Copy link
    Contributor Author

    raising an OSError instead of a logger.error doesn't work somehow for me, works as expected though

    @aseembits93 aseembits93 requested a review from misrasaurabh1 May 21, 2025 02:25
    @aseembits93 aseembits93 disabled auto-merge May 21, 2025 02:25
    @aseembits93
    Copy link
    Contributor Author

    Thanks @KRRT7 for the help!

    @aseembits93 aseembits93 requested a review from KRRT7 May 21, 2025 03:56
    @aseembits93
    Copy link
    Contributor Author

    @KRRT7 ready to merge

    1 similar comment
    @aseembits93
    Copy link
    Contributor Author

    @KRRT7 ready to merge

    @aseembits93 aseembits93 enabled auto-merge May 28, 2025 00:57
    @aseembits93 aseembits93 requested review from KRRT7 and removed request for KRRT7 May 28, 2025 00:57
    Copy link
    Contributor Author

    @aseembits93 aseembits93 left a comment

    Choose a reason for hiding this comment

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

    tested with ruff and black

    @misrasaurabh1 misrasaurabh1 disabled auto-merge May 28, 2025 01:50
    @misrasaurabh1 misrasaurabh1 merged commit 8a60a13 into main May 28, 2025
    16 checks passed
    @aseembits93 aseembits93 deleted the fix-formatter-during-runtime branch May 28, 2025 02:24
    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.

    3 participants