Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented May 29, 2025

PR Type

Enhancement


Description

  • Use format_code to replace subprocess logic

  • Remove manual formatter error handling

  • Import format_code in env_utils module


Changes walkthrough 📝

Relevant files
Enhancement
env_utils.py
Use `format_code` in formatter check                                         

codeflash/code_utils/env_utils.py

  • Added import of format_code function
  • Replaced subprocess loop with format_code call
  • Removed custom FormatterNotFoundError handling
  • +2/-15   

    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

    Missing Error Handling

    check_formatter_installed always returns True and doesn’t capture or propagate failures from format_code. Add error handling to detect and surface formatter invocation errors.

    def check_formatter_installed(formatter_cmds: list[str]) -> bool:
        return_code = True
        if formatter_cmds[0] == "disabled":
            return return_code
        tmp_code = """print("hello world")"""
        with tempfile.NamedTemporaryFile(mode="w", encoding="utf-8", suffix=".py") as f:
            f.write(tmp_code)
            f.flush()
            tmp_file = Path(f.name)
            format_code(formatter_cmds, tmp_file)
        tmp_file.unlink(missing_ok=True)
        return return_code
    Redundant Variable

    The return_code variable is no longer updated after refactoring and could be removed to simplify the function.

    return_code = True

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add error handling around formatter call

    Wrap the call to format_code in a try-except to catch any errors (e.g., missing
    formatter executable or nonzero exit code) and raise a FormatterNotFoundError with a
    clear message. This preserves the original behavior of signaling failure instead of
    allowing unexpected exceptions to bubble up.

    codeflash/code_utils/env_utils.py [32]

    -format_code(formatter_cmds, tmp_file)
    +try:
    +    format_code(formatter_cmds, tmp_file)
    +except Exception as e:
    +    raise FormatterNotFoundError(f"Error running formatter command: {e}")
    Suggestion importance[1-10]: 8

    __

    Why: Wrapping format_code in a try-except preserves the PR’s intention to detect missing or failing formatters and reintroduces raising FormatterNotFoundError, which is crucial for correct error signaling.

    Medium
    General
    Ensure temp file always removed

    Move the cleanup of tmp_file into a finally block to guarantee removal even if
    formatting fails. This prevents orphaned temp files when exceptions occur.

    codeflash/code_utils/env_utils.py [33]

    -tmp_file.unlink(missing_ok=True)
    +try:
    +    format_code(formatter_cmds, tmp_file)
    +except Exception as e:
    +    raise FormatterNotFoundError(f"Error running formatter command: {e}")
    +finally:
    +    tmp_file.unlink(missing_ok=True)
    Suggestion importance[1-10]: 6

    __

    Why: Moving tmp_file.unlink(missing_ok=True) into a finally block improves robustness by guaranteeing cleanup even on exceptions, though it’s a minor enhancement.

    Low

    @aseembits93 aseembits93 requested a review from KRRT7 May 29, 2025 22:20
    @openhands-ai
    Copy link

    openhands-ai bot commented May 29, 2025

    Looks like there are a few issues preventing this PR from being merged!

    • GitHub Actions are failing:
      • end-to-end-test

    If you'd like me to help, just leave a comment, like

    @OpenHands please fix the failing actions on PR #254

    Feel free to include any additional details that might help me get this PR into a better state.

    You can manage your notification settings

    @misrasaurabh1 misrasaurabh1 self-requested a review May 29, 2025 23:02
    @misrasaurabh1 misrasaurabh1 merged commit 8f60ace into main May 29, 2025
    16 checks passed
    @KRRT7 KRRT7 deleted the fix-formatter branch May 30, 2025 00:07
    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