Skip to content

Conversation

@aseembits93
Copy link
Contributor

@aseembits93 aseembits93 commented Nov 11, 2025

User description

fixes https://github.com/codeflash-ai/codeflash/actions/runs/19249933706/job/55032527683?pr=895


PR Type

Enhancement, Tests


Description

  • Centralize import sorting via sort_imports

  • Make sort_imports kwargs-based with error catch

  • Replace direct isort.code usages in tests


Diagram Walkthrough

flowchart LR
  A["formatter.sort_imports(code, **kwargs)"] -- "used by" --> B["instrument_existing_tests inject_* functions"]
  A -- "used by" --> C["tests: instrumentation_run_results_aiservice"]
  D["isort.code direct calls"] -- "replaced with" --> A
Loading

File Walkthrough

Relevant files
Enhancement
formatter.py
Make sort_imports kwargs-based wrapper with fallback         

codeflash/code_utils/formatter.py

  • Change sort_imports to accept **kwargs.
  • Delegate to isort.code(code, **kwargs).
  • Keep broad exception handling with fallback.
  • Update signature and noqa for ANN003.
+2/-2     
instrument_existing_tests.py
Route test instrumentation import sorting via wrapper       

codeflash/code_utils/instrument_existing_tests.py

  • Remove direct isort import.
  • Use sort_imports(..., float_to_top=True) after AST changes.
  • Replace two isort.code calls with sort_imports.
+2/-3     
Tests
test_instrumentation_run_results_aiservice.py
Tests use centralized sort_imports instead of isort.code 

tests/test_instrumentation_run_results_aiservice.py

  • Import sort_imports from formatter.
  • Replace isort.code(...) with sort_imports(...) twice.
  • Retain isort.Config usage passed via kwargs.
+3/-2     

@aseembits93 aseembits93 requested a review from KRRT7 November 11, 2025 00:17
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Codeflash Bot 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 Reviewer Guide 🔍

Here are some key observations to aid the review process:

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

Possible Issue

The new function returns the variable sorted_code on success, but the variable is never returned in the current snippet. Ensure the function returns the sorted result instead of falling through without a return.

def sort_imports(code: str, **kwargs) -> str:  # noqa: ANN003
    try:
        # Deduplicate and sort imports, modify the code in memory, not on disk
        sorted_code = isort.code(code, **kwargs)
    except Exception:  # this will also catch the FileSkipComment exception, use this fn everywhere
        logger.exception("Failed to sort imports with isort.")
        return code  # Fall back to original code if isort fails
Import Missing

The file now uses sort_imports(...) but does not import it. Add from codeflash.code_utils.formatter import sort_imports or equivalent.

    return False, None

# Add necessary imports
new_imports = [ast.Import(names=[ast.alias(name="os")])]
if test_framework == "unittest":
    new_imports.append(ast.Import(names=[ast.alias(name="timeout_decorator")]))

tree.body = [*new_imports, *tree.body]
return True, sort_imports(ast.unparse(tree), float_to_top=True)

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Return sorted result explicitly

Ensure the function consistently returns the sorted result by adding an explicit
return of sorted_code. Without it, the function returns None on success, breaking
callers that expect a string.

codeflash/code_utils/formatter.py [166-172]

 def sort_imports(code: str, **kwargs) -> str:  # noqa: ANN003
     try:
         # Deduplicate and sort imports, modify the code in memory, not on disk
         sorted_code = isort.code(code, **kwargs)
+        return sorted_code
     except Exception:  # this will also catch the FileSkipComment exception, use this fn everywhere
         logger.exception("Failed to sort imports with isort.")
         return code  # Fall back to original code if isort fails
Suggestion importance[1-10]: 9

__

Why: The new function computes sorted_code but never returns it; adding return sorted_code fixes a clear bug that would otherwise return None, directly impacting callers. The suggested change precisely matches the new hunk context and is correct.

High
General
Use config for isort option

Pass float_to_top via config to align with isort.code's supported parameters when
forwarding kwargs. This prevents unexpected TypeError if float_to_top isn't accepted
directly in some configurations.

codeflash/code_utils/instrument_existing_tests.py [743]

-return True, sort_imports(ast.unparse(tree), float_to_top=True)
+return True, sort_imports(ast.unparse(tree), config=isort.Config(float_to_top=True))
Suggestion importance[1-10]: 6

__

Why: Passing float_to_top via config=isort.Config(...) is safer and consistent with other usages in the PR, reducing risk of parameter mismatches. However, given sort_imports forwards kwargs to isort.code, the current code likely works, so this is a moderate improvement.

Low
Ensure config symbol availability

Import isort.Config where used or construct the config upstream to avoid relying on
an unimported symbol in test scope. Alternatively, forward a plain dict and let
sort_imports build the config internally.

tests/test_instrumentation_run_results_aiservice.py [119-121]

+instrumented_behavior_test_source = sort_imports(
+    instrumented_behavior_test_source, config=isort.Config(float_to_top=True)
+)
 
-
Suggestion importance[1-10]: 2

__

Why: The test file already imports isort, so isort.Config is available; the improved code is identical to the existing code, offering no real change. At most, it reminds to ensure the import exists, which has low impact here.

Low

KRRT7
KRRT7 previously approved these changes Nov 11, 2025
@aseembits93 aseembits93 merged commit 827cf36 into main Nov 11, 2025
20 of 23 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