Skip to content

Conversation

@mohammedahmed18
Copy link
Contributor

@mohammedahmed18 mohammedahmed18 commented Oct 21, 2025

PR Type

Enhancement


Description

  • Migrate to pygls v2 API

  • Replace uri handling with workspace document

  • Update logging to use protocol.notify

  • Bump dependency to pygls v2


Diagram Walkthrough

flowchart LR
  deps["pyproject: pygls >=2,<3"]
  uris["Remove uris.to_fs_path usage"]
  docs["Use workspace.get_text_document"]
  log["Use protocol.notify for logs"]
  paths["Derive Path from document.path"]

  deps -- "runtime requirement" --> server["LSP Server"]
  uris -- "v2 migration" --> docs
  docs -- "retrieve document" --> paths
  log -- "v2 API change" --> server
Loading

File Walkthrough

Relevant files
Enhancement
beta.py
Switch to workspace document-based path handling                 

codeflash/lsp/beta.py

  • Stop using uris.to_fs_path.
  • Get document via server.workspace.get_text_document.
  • Use document.path to build Path and relatives.
  • Update log message to use document URI.
+10/-6   
server.py
Update LanguageServer imports and notify API                         

codeflash/lsp/server.py

  • Import LanguageServer from pygls.lsp.server.
  • Use self.protocol.notify for window/logMessage.
+2/-2     
Dependencies
pyproject.toml
Pin dependency to pygls v2 range                                                 

pyproject.toml

  • Bump pygls requirement to >=2.0.0,<3.0.0.
+1/-1     

@mohammedahmed18 mohammedahmed18 requested review from KRRT7 and Saga4 and removed request for KRRT7 October 21, 2025 01:29
@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

Path handling

Switching from URI to workspace document path is correct for pygls v2, but ensure that Path(document.path) is always absolute and normalized across platforms; relative_to(project_root) may raise if roots differ (e.g., symlinks or case differences on Windows). Validate that document.path aligns with original_args.project_root and add defensive error handling if not.

document_uri = params.textDocument.uri
document = server.workspace.get_text_document(document_uri)

file_path = Path(document.path)

if not server.optimizer:
    return {"status": "error", "message": "optimizer not initialized"}

server.optimizer.args.file = file_path
server.optimizer.args.function = None  # Always get ALL functions, not just one
server.optimizer.args.previous_checkpoint_functions = False
Logging API usage

Using protocol.notify("window/logMessage", ...) matches v2, but confirm protocol is initialized at call time and consider guarding against None or disconnected transport to avoid runtime errors during early initialization or shutdown.

lsp_message_type = type_mapping.get(message_type, MessageType.Info)

# Send log message to client (appears in output channel)
log_params = LogMessageParams(type=lsp_message_type, message=message)
self.protocol.notify("window/logMessage", log_params)

@github-actions
Copy link

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Add safe document retrieval

Guard against missing documents in the workspace; get_text_document may return None
or raise if the document isn't open. Fallback to the URI filesystem path to avoid
crashes.

codeflash/lsp/beta.py [112-116]

 document_uri = params.textDocument.uri
-document = server.workspace.get_text_document(document_uri)
+try:
+    document = server.workspace.get_text_document(document_uri)
+    file_path = Path(document.path)
+except Exception:
+    file_path = Path(document_uri.replace("file://", ""))
 
-file_path = Path(document.path)
-
Suggestion importance[1-10]: 6

__

Why: Adding a try/except around get_text_document can prevent crashes when the document isn't open, and the improved code aligns with the surrounding PR changes. It's a minor robustness improvement with moderate value.

Low
Possible issue
Use server notify API

In pygls v2, notifications are sent via the server API, not via the protocol
attribute. Using self.protocol.notify may fail; route notifications through the
language server instance instead.

codeflash/lsp/server.py [59]

-self.protocol.notify("window/logMessage", log_params)
+self.notify("window/logMessage", log_params)
Suggestion importance[1-10]: 3

__

Why: The suggestion is reasonable stylistically, but the PR intentionally switched to self.protocol.notify, which is valid in pygls v2. Changing to self.notify is not necessary and may not be accurate depending on the server wrapper; minimal impact.

Low

@mohammedahmed18 mohammedahmed18 merged commit 4066c9a into main Oct 21, 2025
23 of 24 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