Skip to content

Conversation

@alvin-r
Copy link
Contributor

@alvin-r alvin-r commented Mar 14, 2025

User description

… and makes it easier to search for matches when replacing code.


PR Type

  • Enhancement

Description

  • Switch preexisting objects from list to set.

  • Use tuple for immutable function parent info.

  • Refactor type annotations in core utilities.

  • Update tests to reflect new data structures.


Changes walkthrough 📝

Relevant files
Enhancement
code_extractor.py
Update find_preexisting_objects to return set.                     

codeflash/code_utils/code_extractor.py

  • Changed return type from list to set.
  • Wrapped function parent info in tuples.
  • +6/-6     
    code_replacer.py
    Refactor preexisting_objects type in code replacer.           

    codeflash/code_utils/code_replacer.py

  • Modified type annotations for preexisting_objects.
  • Replaced list usage with sets and tuple comparisons.
  • +7/-7     
    code_context_extractor.py
    Deduplicate preexisting objects in context extraction.     

    codeflash/context/code_context_extractor.py

  • Converted preexisting_objects collection to set union.
  • Removed duplicate entries using set conversion.
  • +3/-3     
    Tests
    test_code_replacement.py
    Adjust tests for updated preexisting_objects type.             

    tests/test_code_replacement.py

  • Updated preexisting_objects type to set of tuples.
  • Replaced list concatenations with set unions.
  • +16/-16 

    Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • alvin-r added 2 commits March 13, 2025 18:52
    … and makes it easier to search for matches when replacing code.
    misrasaurabh1
    misrasaurabh1 previously approved these changes Mar 14, 2025
    @github-actions
    Copy link

    PR Reviewer Guide 🔍

    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

    Type Conversion

    The new function find_preexisting_objects returns a set of tuples instead of a list of tuples. Ensure that downstream code expecting an ordered collection is not adversely affected and that the change in data structure meets all usage requirements.

    def find_preexisting_objects(source_code: str) -> set[tuple[str, tuple[FunctionParent, ...]]]:
        """Find all preexisting functions, classes or class methods in the source code."""
        preexisting_objects: set[tuple[str, tuple[FunctionParent, ...]]] = set()
        try:
            module_node: ast.Module = ast.parse(source_code)
        except SyntaxError:
            logger.exception("find_preexisting_objects - Syntax error while parsing code")
            return preexisting_objects
        for node in module_node.body:
            if isinstance(node, (ast.FunctionDef, ast.AsyncFunctionDef)):
                preexisting_objects.add((node.name, ()))
            elif isinstance(node, ast.ClassDef):
                preexisting_objects.add((node.name, ()))
                for cnode in node.body:
                    if isinstance(cnode, (ast.FunctionDef, ast.AsyncFunctionDef)):
                        preexisting_objects.add((cnode.name, (FunctionParent(node.name, "ClassDef"),)))
        return preexisting_objects
    Logic Check

    In the updated visit_FunctionDef method, the condition now checks for (node.name.value, ()) not in self.preexisting_objects. Please verify that this change correctly distinguishes new functions and that the tuple structure (using an empty tuple for functions without parents) is handled consistently across all cases.

    elif (
        self.preexisting_objects
        and (node.name.value, ()) not in self.preexisting_objects
        and self.current_class is None
    ):
    Test Consistency

    The tests now use set unions (e.g., using | instead of list concatenation) for merging preexisting objects. Confirm that this change properly removes duplicates without affecting the intended order or test expectations.

    """
        preexisting_objects = find_preexisting_objects(original_code_main) | find_preexisting_objects(original_code_helper)
        new_main_code: str = replace_functions_and_add_imports(

    @github-actions
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use proper empty set initializer

    Replace the empty list literal with a proper empty set initializer.

    tests/test_code_replacement.py [857]

    -preexisting_objects: set[tuple[str, tuple[FunctionParent,...]]] = []
    +preexisting_objects: set[tuple[str, tuple[FunctionParent,...]]] = set()
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly fixes a type mismatch by replacing an empty list with an empty set initializer, ensuring consistency with the declared type.

    Medium
    General
    Use direct set conversion

    Remove the unnecessary list conversion and use the set directly.

    codeflash/context/code_context_extractor.py [69-74]

    -preexisting_objects = list(set(
    +preexisting_objects = set(
          chain(
              find_preexisting_objects(final_read_writable_code),
              *(find_preexisting_objects(codestring.code) for codestring in read_only_code_markdown.code_strings),
          )
    -))
    +)
    Suggestion importance[1-10]: 5

    __

    Why: Removing the unnecessary conversion to a list simplifies the code and aligns with the expected set type, though it is a minor improvement in clarity.

    Low

    @alvin-r alvin-r merged commit a0afba2 into main Mar 14, 2025
    15 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