Skip to content

Conversation

@Saga4
Copy link
Contributor

@Saga4 Saga4 commented Aug 12, 2025

PR Type

Enhancement


Description

  • Log warning when formatter command not found

  • Catch FileNotFoundError during diff formatting

  • Catch FileNotFoundError in main formatting call

  • Return original code if formatter missing


Diagram Walkthrough

flowchart LR
  A["apply_formatter_cmds"] -->|formatter missing| B["Log warning"]
  B --> C{"exit_on_failure?"}
  C -->|yes| D["Raise exception"]
  C -->|no| E["Continue without formatting"]
  F["format_code: diff stage"] -->|apply_formatter_cmds| G{"success?"}
  G -->|no| H["Log warning & return original"]
  G -->|yes| I["Generate & check diff"]
  J["format_code: main stage"] -->|apply_formatter_cmds| K{"success?"}
  K -->|no| H
  K -->|yes| L["Log debug & return formatted code"]
Loading

File Walkthrough

Relevant files
Error handling
formatter.py
Handle missing formatter with logging                                       

codeflash/code_utils/formatter.py

  • Added warning when formatter command not found
  • Wrapped diff formatting in try/except FileNotFoundError
  • Wrapped main formatting call in try/except FileNotFoundError
  • Return original code if formatter missing
+33/-17 

@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

Early Return Bug

The return in the diff-check exception block appears mis-indented, causing an early exit and skipping the main formatting stage whenever the formatter is missing.

except FileNotFoundError as e:
    logger.warning(f"Formatter not found, skipping diff check: {e}")
    # Continue without formatting checks
    return original_code
Diff Stage Flow

Catching FileNotFoundError during diff formatting immediately returns the original code instead of skipping diff checks and continuing to the main formatting call.

except FileNotFoundError as e:
    logger.warning(f"Formatter not found, skipping diff check: {e}")
    # Continue without formatting checks
    return original_code

@CLAassistant
Copy link

CLAassistant commented Aug 12, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ mohammedahmed18
❌ saga4


saga4 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Return formatted code when acceptable

Only return the original code when the diff is too large or the formatter is
missing. Otherwise, return the newly formatted code after a successful diff check to
avoid skipping formatting unconditionally.

codeflash/code_utils/formatter.py [132-154]

 try:
     formatted_temp, formatted_code = apply_formatter_cmds(
         formatter_cmds, original_temp, test_dir_str, print_status=False
     )
-    diff_output = generate_unified_diff(
-        original_code_without_opfunc,
-        formatted_code,
-        from_file=str(original_temp),
-        to_file=str(formatted_temp),
-    )
-    diff_lines_count = get_diff_lines_count(diff_output)
-    max_diff_lines = min(int(original_code_lines * 0.3), 50)
-    if diff_lines_count > max_diff_lines and max_diff_lines != -1:
-        logger.debug(
-            f"Skipping formatting {path}: {diff_lines_count} lines would change (max: {max_diff_lines})"
-        )
 except FileNotFoundError as e:
     logger.warning(f"Formatter not found, skipping diff check: {e}")
-    # Continue without formatting checks
-return original_code
+    return original_code
 
+diff_output = generate_unified_diff(
+    original_code_without_opfunc,
+    formatted_code,
+    from_file=str(original_temp),
+    to_file=str(formatted_temp),
+)
+diff_lines_count = get_diff_lines_count(diff_output)
+max_diff_lines = min(int(original_code_lines * 0.3), 50)
+
+if diff_lines_count > max_diff_lines and max_diff_lines != -1:
+    logger.debug(
+        f"Skipping formatting {path}: {diff_lines_count} lines would change (max: {max_diff_lines})"
+    )
+    return original_code
+
+return formatted_code
+
Suggestion importance[1-10]: 9

__

Why: The current diff-check block always returns the original code even when the diff is acceptable; this change correctly returns formatted_code only when appropriate, fixing a functional bug.

High

@Saga4 Saga4 requested a review from mohammedahmed18 August 12, 2025 14:16
@Saga4 Saga4 requested a review from mohammedahmed18 August 16, 2025 23:16
@Saga4 Saga4 merged commit a2825f9 into main Aug 17, 2025
18 of 19 checks passed
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.

4 participants