Skip to content

Conversation

@KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Apr 12, 2025

PR Type

Enhancement, Tests


Description

  • Introduce format_code_in_memory using black formatting

  • Replace sort_imports calls across modules

  • Update tests with new formatter assertions

  • Add black dependency and CI improvements


Changes walkthrough 📝

Relevant files
Enhancement
4 files
formatter.py
Add format_code_in_memory with black and isort integration
+18/-7   
instrument_existing_tests.py
Refactor instrumentation to use new formatter function     
+2/-4     
function_optimizer.py
Replace sort_imports with format_code_in_memory usage       
+3/-3     
unit-tests.yaml
Adjust CI test command with warnings disabled                       
+1/-1     
Tests
3 files
test_formatter.py
Update formatter tests to use new function call                   
+7/-7     
test_instrument_all_and_run.py
Update instrumentation tests to compare formatted code     
+7/-4     
test_instrument_tests.py
Revise instrumentation tests with formatter updates           
+198/-89
Dependencies
1 files
pyproject.toml
Add black dependency for consistent code formatting           
+1/-0     

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 github-actions bot added workflow-modified This PR modifies GitHub Actions workflows Review effort 4/5 labels Apr 12, 2025
    @github-actions
    Copy link

    github-actions bot commented Apr 12, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 75763b4)

    Here are some key observations to aid the review process:

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

    Formatting Consistency

    The new function format_code_in_memory combines black formatting with import sorting via isort.partial. Please verify that the merged formatting behavior produces consistent and predictable results, especially as black evolves.

    def format_code_in_memory(code: str, *, imports_only: bool = False) -> str:
        if imports_only:
            try:
                sorted_code = imports_sort(code)
            except Exception:  # noqa: BLE001
                logger.debug("Failed to sort imports with isort.")
                return code
            return sorted_code
        try:
            formatted_code = black.format_str(code, mode=black.FileMode())
            formatted_code = imports_sort(formatted_code)
        except Exception:  # noqa: BLE001
            logger.debug("Failed to format code with black.")
            return code
    Fragile Test Assertions

    Test assertions now compare code formatted with format_code_in_memory. Since black's output may vary with version or configuration, consider using more robust checks than raw string equality.

        new_code = format_code_in_memory(original_code, imports_only=True)
        assert new_code == "import os\n"
    
    
    def test_remove_multiple_duplicate_imports():
        """Test that multiple duplicate imports are removed when should_sort_imports is True."""
        original_code = "import sys\nimport os\nimport sys\n"
    
        new_code = format_code_in_memory(original_code, imports_only=True)
        assert new_code == "import os\nimport sys\n"
    
    
    def test_sorting_imports():
        """Test that imports are sorted when should_sort_imports is True."""
        original_code = "import sys\nimport unittest\nimport os\n"
    
        new_code = format_code_in_memory(original_code, imports_only=True)
        assert new_code == "import os\nimport sys\nimport unittest\n"

    @github-actions
    Copy link

    github-actions bot commented Apr 12, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Separate error handling blocks

    Separate the black formatting and import sorting into distinct try blocks so that
    failures in import sorting are logged and handled appropriately.

    codeflash/code_utils/formatter.py [61-66]

     try:
         formatted_code = black.format_str(code, mode=black.FileMode())
    -    formatted_code = imports_sort(formatted_code)
     except Exception:  # noqa: BLE001
         logger.debug("Failed to format code with black.")
         return code
    +try:
    +    formatted_code = imports_sort(formatted_code)
    +except Exception:  # noqa: BLE001
    +    logger.debug("Failed to sort imports with isort after formatting.")
    +    return formatted_code
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion improves error diagnostics by separating black formatting errors from import sorting failures, enhancing clarity; however, the benefit is moderate and introduces some additional complexity.

    Low

    @KRRT7 KRRT7 marked this pull request as ready for review April 12, 2025 12:07
    @github-actions
    Copy link

    Persistent review updated to latest commit 75763b4

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Separate error handling blocks

    Separate the black formatting and import sorting into distinct try blocks so that
    failures in import sorting are logged and handled appropriately.

    codeflash/code_utils/formatter.py [61-66]

     try:
         formatted_code = black.format_str(code, mode=black.FileMode())
    -    formatted_code = imports_sort(formatted_code)
     except Exception:  # noqa: BLE001
         logger.debug("Failed to format code with black.")
         return code
    +try:
    +    formatted_code = imports_sort(formatted_code)
    +except Exception:  # noqa: BLE001
    +    logger.debug("Failed to sort imports with isort after formatting.")
    +    return formatted_code
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion improves error diagnostics by separating black formatting errors from import sorting failures, enhancing clarity; however, the benefit is moderate and introduces some additional complexity.

    Low

    Saga4
    Saga4 previously approved these changes Apr 12, 2025
    Copy link
    Contributor

    @Saga4 Saga4 left a comment

    Choose a reason for hiding this comment

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

    LGTM

    @KRRT7
    Copy link
    Contributor Author

    KRRT7 commented Apr 12, 2025

    @Saga4 any idea why they're "expected"?

    @KRRT7 KRRT7 enabled auto-merge April 12, 2025 12:44
    @KRRT7 KRRT7 changed the title fix failing tests due to fstrings mismatch fix failing tests due to fstrings mismatch & loosen pytest requirements Apr 12, 2025
    crosshair-tool = ">=0.0.78"
    coverage = ">=7.6.4"
    line_profiler=">=4.2.0" #this is the minimum version which supports python 3.13
    black = "^25.1.0"
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    we stopped requiring black since many users don't use black. Why do we require black, and not have it as an option?

    Copy link
    Contributor Author

    Choose a reason for hiding this comment

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

    this let's us not worry about fstring mismatch across all python versions, and instead let's black take care of it.

    @KRRT7 KRRT7 closed this Apr 21, 2025
    auto-merge was automatically disabled April 21, 2025 11:13

    Pull request was closed

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    Review effort 4/5 workflow-modified This PR modifies GitHub Actions workflows

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    5 participants