Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Nov 10, 2025

PR Type

Enhancement


Description

  • Use configured values as LSP defaults

  • Parse chained formatter commands with '&&'

  • Preserve formatter cmds from list or string

  • Keep suggestions choices unchanged


Diagram Walkthrough

flowchart LR args["Server args (configured values)"] suggestions["Computed suggestions (choices, computed defaults)"] normalize["Normalize formatter cmds (join ' && ')"] response["LSP config suggestions (defaults prefer configured)"] args -- "module_root/tests_root/test_framework/formatter_cmds" --> response suggestions -- "choices" --> response args -- "formatter_cmds list or string" --> normalize normalize -- "configured formatter default" --> response 
Loading

File Walkthrough

Relevant files
Enhancement
cmd_init.py
Support chained formatter commands in init                             

codeflash/cli_cmds/cmd_init.py

  • Split formatter command by ' && ' into list
  • Return multiple commands when chained
+2/-0     
beta.py
Use configured settings as LSP defaults                                   

codeflash/lsp/beta.py

  • Prefer configured values as LSP default suggestions
  • Normalize formatter cmds from list or string
  • Keep choices from suggestion generator
+18/-4   

@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

Parsing Robustness

Splitting formatter strings only on the substring " && " fails if users omit spaces around && or include multiple spaces; consider normalizing whitespace or splitting on '&&' with surrounding whitespace trimmed.

if " && " in formatter: return formatter.split(" && ") return [formatter]
Path Handling

Using Path(...).relative_to(Path.cwd()) will raise a ValueError if the configured path is not under CWD; consider fallbacks like os.path.relpath with try/except or returning the absolute path.

configured_module_root = Path(server.args.module_root).relative_to(Path.cwd()) if server.args.module_root else None configured_tests_root = Path(server.args.tests_root).relative_to(Path.cwd()) if server.args.tests_root else None configured_test_framework = server.args.test_framework if server.args.test_framework else None
Type Normalization

The LSP default for formatter_cmds is a joined string, but choices likely expect a list; ensure downstream consumers handle the string default consistently with list-based choices.

configured_formatter = "" if isinstance(server.args.formatter_cmds, list): configured_formatter = " && ".join([cmd.strip() for cmd in server.args.formatter_cmds]) elif isinstance(server.args.formatter_cmds, str): configured_formatter = server.args.formatter_cmds.strip() return { "module_root": {"choices": module_root_suggestions, "default": configured_module_root or default_module_root}, "tests_root": {"choices": tests_root_suggestions, "default": configured_tests_root or default_tests_root}, "test_framework": { "choices": test_framework_suggestions, "default": configured_test_framework or default_test_framework, }, "formatter_cmds": {"choices": formatter_suggestions, "default": configured_formatter or default_formatter}, }
@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Safely compute relative paths

Guard against non-subpath values to prevent ValueError from Path.relative_to when
given absolute paths outside cwd or relative paths with "..". Fallback to the
original string if not relative to cwd.

codeflash/lsp/beta.py [209-211]

-configured_module_root = Path(server.args.module_root).relative_to(Path.cwd()) if server.args.module_root else None -configured_tests_root = Path(server.args.tests_root).relative_to(Path.cwd()) if server.args.tests_root else None +def _rel_to_cwd(p: str | None): + if not p: + return None + try: + return Path(p).resolve().relative_to(Path.cwd().resolve()) + except Exception: + return p +configured_module_root = _rel_to_cwd(server.args.module_root) +configured_tests_root = _rel_to_cwd(server.args.tests_root)
Suggestion importance[1-10]: 8

__

Why: Using Path.relative_to can raise ValueError for paths outside cwd; adding a safe helper prevents runtime errors and preserves intended behavior, addressing a real edge case.

Medium
Trim split formatter commands

Trim each split command to avoid leading/trailing spaces that can break execution
equality checks. This ensures consistent behavior regardless of user spacing around
the delimiter.

codeflash/cli_cmds/cmd_init.py [1008-1009]

 if " && " in formatter: - return formatter.split(" && ") + return [cmd.strip() for cmd in formatter.split(" && ")]
Suggestion importance[1-10]: 6

__

Why: Trimming each split command improves robustness against user spacing and aligns with similar trimming elsewhere; it's correct and low-risk but a minor enhancement.

Low
General
Ignore empty formatter configs

Normalize empty or whitespace-only formatter values to None to avoid setting a
misleading empty default. This preserves fallback to the detected default when no
valid configuration is provided.

codeflash/lsp/beta.py [213-218]

-configured_formatter = "" +configured_formatter = None if isinstance(server.args.formatter_cmds, list): - configured_formatter = " && ".join([cmd.strip() for cmd in server.args.formatter_cmds]) + cleaned = [cmd.strip() for cmd in server.args.formatter_cmds if cmd and cmd.strip()] + configured_formatter = " && ".join(cleaned) if cleaned else None elif isinstance(server.args.formatter_cmds, str): - configured_formatter = server.args.formatter_cmds.strip() + cleaned = server.args.formatter_cmds.strip() + configured_formatter = cleaned if cleaned else None
Suggestion importance[1-10]: 7

__

Why: Normalizing empty values to None ensures fallback to defaults and avoids misleading empty strings; it's consistent with nearby logic and improves configuration handling.

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

2 participants