Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Aug 26, 2025

needed for https://github.com/codeflash-ai/codeflash-internal/pull/1761
The lsp client needs to know what is the file for each function to be able to send the optimization request with --file


Description

  • Group functions by file in current diff

  • Change return format to file-to-functions dict

  • Update return type annotation


File Walkthrough

Relevant files
Enhancement
beta.py
Group functions by file path                                                         

codeflash/lsp/beta.py

  • Adjusted get_functions_in_current_git_diff return structure
  • Replaced flat list of functions with file-to-function mapping
  • Updated return type annotation accordingly
+5/-3     

@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

Type Annotation

The return type annotation is too generic and doesn’t explicitly define the expected keys and value types. Consider defining and using a TypedDict to represent the structured response with keys "functions" and "status" for improved clarity and type safety.

) -> dict[str, str | dict[str, list[str]]]:
    functions = get_functions_within_git_diff(uncommitted_changes=True)
Missing Tests

There are no tests added or updated to verify the new grouped-by-file diff functionality. It would be beneficial to add unit tests covering typical and edge cases for get_functions_in_current_git_diff.

file_to_qualified_names: dict[str, list[str]] = {
    str(path): [f.qualified_name for f in funcs] for path, funcs in file_to_funcs_to_optimize.items()
}
return {"functions": file_to_qualified_names, "status": "success"}

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Sort qualified names lists

Sort the lists of function names so that the output is deterministic and easier to
test. This ensures consistent ordering across runs.

codeflash/lsp/beta.py [58-60]

 file_to_qualified_names: dict[str, list[str]] = {
-    str(path): [f.qualified_name for f in funcs] for path, funcs in file_to_funcs_to_optimize.items()
+    str(path): sorted(f.qualified_name for f in funcs) for path, funcs in file_to_funcs_to_optimize.items()
 }
Suggestion importance[1-10]: 7

__

Why: Sorting the list from [f.qualified_name for f in funcs] yields deterministic output for functions results, aiding reproducible tests and debugging.

Medium
Normalize file path format

Convert file path keys to POSIX format using path.as_posix() to ensure consistent
separators across platforms. This avoids OS-specific path issues when serializing to
JSON.

codeflash/lsp/beta.py [58-60]

 file_to_qualified_names: dict[str, list[str]] = {
-    str(path): [f.qualified_name for f in funcs] for path, funcs in file_to_funcs_to_optimize.items()
+    path.as_posix(): [f.qualified_name for f in funcs] for path, funcs in file_to_funcs_to_optimize.items()
 }
Suggestion importance[1-10]: 6

__

Why: Using path.as_posix() ensures consistent path separators across platforms, improving JSON serialization portability without altering functionality.

Low

@mohammedahmed18 mohammedahmed18 requested a review from KRRT7 August 28, 2025 11:45
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.

LG, whats the dependent PR on client side for this. Could you please tag it in description.

@mohammedahmed18 mohammedahmed18 merged commit fdaf6c0 into main Sep 1, 2025
19 of 20 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.

2 participants