Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented May 31, 2025

PR Type

Enhancement


Description

  • Add capture_output parameter to format_code

  • Suppress output during formatter install check

  • Conditional console.rule based on capture_output

  • Update check_formatter_installed call parameters


Changes walkthrough 📝

Relevant files
Enhancement
env_utils.py
Suppress formatter output in install check                             

codeflash/code_utils/env_utils.py

  • Include capture_output=False in format_code call
  • Prevent output during formatter installation check
  • +1/-1     
    formatter.py
    Add capture_output option to format_code                                 

    codeflash/code_utils/formatter.py

  • Add capture_output parameter to format_code signature
  • Wrap console.rule call under capture_output condition
  • +3/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @aseembits93 aseembits93 marked this pull request as draft May 31, 2025 02:40
    @github-actions
    Copy link

    github-actions bot commented May 31, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 173bc9d)

    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

    Missing tests

    No tests have been added or updated to cover the new capture_output parameter in format_code. Add tests to verify both suppressed output and console rule display when enabled.

    def format_code(formatter_cmds: list[str], path: Path, capture_output: bool = True) -> str:  # noqa
        # TODO: Only allow a particular whitelist of formatters here to prevent arbitrary code execution
        formatter_name = formatter_cmds[0].lower()
        if not path.exists():
            msg = f"File {path} does not exist. Cannot format the file."
            raise FileNotFoundError(msg)
        if formatter_name == "disabled":
            return path.read_text(encoding="utf8")
        file_token = "$file"  # noqa: S105
        for command in formatter_cmds:
            formatter_cmd_list = shlex.split(command, posix=os.name != "nt")
            formatter_cmd_list = [path.as_posix() if chunk == file_token else chunk for chunk in formatter_cmd_list]
            try:
                result = subprocess.run(formatter_cmd_list, capture_output=True, check=False)
                if result.returncode == 0:
                    if capture_output:
                        console.rule(f"Formatted Successfully with: {formatter_name.replace('$file', path.name)}")
                else:
    Console.rule formatting

    The console.rule message uses formatter_name.replace('$file', path.name), but formatter_name may not contain $file. Ensure the displayed output correctly reflects the formatter command and target file.

    if capture_output:
        console.rule(f"Formatted Successfully with: {formatter_name.replace('$file', path.name)}")

    @github-actions
    Copy link

    github-actions bot commented May 31, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 173bc9d
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Align subprocess capture with flag

    Respect the capture_output parameter by passing it to subprocess.run, so that when
    capture_output=False the external formatter’s output is truly suppressed rather than
    always being captured.

    codeflash/code_utils/formatter.py [29]

    -result = subprocess.run(formatter_cmd_list, capture_output=True, check=False)
    +result = subprocess.run(formatter_cmd_list, capture_output=capture_output, check=False)
    Suggestion importance[1-10]: 8

    __

    Why: Respecting the capture_output flag in subprocess.run ensures that when capture_output=False the formatter’s output isn’t unnecessarily captured, matching the intended behavior of the format_code API.

    Medium
    Raise on failure in silent mode

    When capture_output=False, raise an exception on formatter failure so that
    higher-level callers like check_formatter_installed can detect and handle missing or
    broken formatters instead of silently logging errors.

    codeflash/code_utils/formatter.py [33-34]

     else:
    -    logger.error(f"Failed to format code with {' '.join(formatter_cmd_list)}")
    +    if capture_output:
    +        logger.error(f"Failed to format code with {' '.join(formatter_cmd_list)}")
    +    else:
    +        raise Exception(f"Failed to format code with {' '.join(formatter_cmd_list)}")
    Suggestion importance[1-10]: 7

    __

    Why: Raising an exception when capture_output=False allows higher‐level callers like check_formatter_installed to detect failures instead of silently logging, improving error propagation and handling.

    Medium

    Previous suggestions

    Suggestions up to commit f9c2926
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Return formatted code output

    The format_code function must still return a string as declared. After running the
    subprocess, decode and return stdout when capture_output=True, and fall back to
    reading the file on disk otherwise.

    codeflash/code_utils/formatter.py [29-33]

     result = subprocess.run(formatter_cmd_list, capture_output=capture_output, check=False)
    +if result.returncode == 0:
    +    console.rule(f"Formatted Successfully with: {formatter_name.replace('$file', path.name)}")
    +    if capture_output:
    +        return result.stdout.decode("utf-8")
    +    return path.read_text(encoding="utf-8")
    Suggestion importance[1-10]: 9

    __

    Why: The format_code function is declared to return a string but currently never returns the formatted output; returning stdout or file contents fixes this critical bug.

    High
    Suppress formatter check output

    The flag is inverted: setting capture_output=False streams formatter output to the
    console. Switch to capture_output=True (or redirect to subprocess.DEVNULL) to
    suppress any output from the formatter during the installation check.

    codeflash/code_utils/env_utils.py [25]

    -format_code(formatter_cmds, tmp_file, capture_output=False)
    +format_code(formatter_cmds, tmp_file, capture_output=True)
    Suggestion importance[1-10]: 6

    __

    Why: Using capture_output=False streams formatter output to the console in check_formatter_installed; capturing it improves UX by suppressing irrelevant formatter logs.

    Low

    @aseembits93 aseembits93 marked this pull request as ready for review May 31, 2025 02:47
    @github-actions
    Copy link

    Persistent review updated to latest commit 173bc9d

    @aseembits93 aseembits93 requested a review from misrasaurabh1 May 31, 2025 02:56
    @aseembits93 aseembits93 enabled auto-merge June 2, 2025 17:10
    @aseembits93 aseembits93 merged commit bfbb733 into main Jun 2, 2025
    16 checks passed
    @aseembits93 aseembits93 deleted the formatter-output-fix branch June 2, 2025 17:17
    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