Skip to content

Add minimal param LSP server example#109

Closed
MarcSkovMadsen wants to merge 1 commit into
mainfrom
enhancement/lsp
Closed

Add minimal param LSP server example#109
MarcSkovMadsen wants to merge 1 commit into
mainfrom
enhancement/lsp

Conversation

@MarcSkovMadsen

@MarcSkovMadsen MarcSkovMadsen commented Jan 18, 2026

Copy link
Copy Markdown
Owner

Just vibe code a param LSP server to better understand what is possible and what it could help with.

The extension installed

image

Hover over class attribute provides parameter details

image

Hover over instance attribute provides parameter details

image

Copilot AI review requested due to automatic review settings January 18, 2026 04:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a proof-of-concept Language Server Protocol (LSP) implementation for param.Parameterized classes, providing IDE features like hover documentation, autocompletion, and diagnostics for param parameters in VS Code.

Changes:

  • Implements a minimal LSP server in Python using pygls that provides hover, completion, and diagnostic features for param types
  • Creates a VS Code extension client to connect to the LSP server
  • Adds comprehensive tests and example files demonstrating the functionality

Reviewed changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 12 comments.

Show a summary per file
File Description
examples/param-lsp/param_lsp.py Core LSP server implementation with hover, completion, and diagnostic features
examples/param-lsp/test_param_lsp.py Unit tests for LSP server functions
examples/param-lsp/example_parameterized.py Example parameterized classes for testing LSP features
examples/param-lsp/README.md Comprehensive documentation covering installation, usage, and limitations
examples/param-lsp/vscode-client/package.json VS Code extension manifest
examples/param-lsp/vscode-client/extension.js VS Code extension entry point that launches the LSP server
.pre-commit-config.yaml Restricts eslint to holoviz_mcp directory
.gitignore Adds ignore patterns for node_modules and extension build artifacts

Comment on lines +58 to +69
def parse_param_args(args_str: str) -> dict:
"""Parse parameter arguments from the declaration string."""
result = {}
# Simple parsing for common kwargs
for match in re.finditer(r"(\w+)\s*=\s*([^,]+?)(?:,|$)", args_str):
key, value = match.groups()
result[key.strip()] = value.strip()
# Check for positional default (first arg without =)
first_arg = args_str.split(",")[0].strip() if args_str else ""
if first_arg and "=" not in first_arg:
result["default"] = first_arg
return result

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The parse_param_args function uses a simple regex that doesn't handle nested parentheses, tuples, or complex values correctly. For example, bounds=(0, 100) will be parsed as bounds='(0' (as acknowledged in test line 38). Consider using a more robust parser like ast.literal_eval or a proper tokenizer to handle nested structures.

Copilot uses AI. Check for mistakes.
Comment on lines +87 to +90
# Check if cursor is within the match
start = match.start()
end = match.end()
if not (start <= position.character <= end):

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The condition check only verifies if the cursor position is within the match, but the match object already confirms the line matches. The character position check should ensure the cursor is specifically on the parameter name or type, not just anywhere in the declaration. Consider refining this to provide more accurate hover positions.

Suggested change
# Check if cursor is within the match
start = match.start()
end = match.end()
if not (start <= position.character <= end):
# Only trigger when cursor is on the parameter name or type, not anywhere in the declaration
name_start, name_end = match.span(1)
type_start, type_end = match.span(2)
if not (
name_start <= position.character <= name_end
or type_start <= position.character <= type_end
):

Copilot uses AI. Check for mistakes.
Comment on lines +171 to +177
def find_instance_class(document: TextDocument, var_name: str) -> str | None:
"""Find the class name for a variable assignment like 'person = Person(...)'."""
for line in document.source.split("\n"):
match = INSTANCE_PATTERN.match(line)
if match and match.group(1) == var_name:
return match.group(2)
return None

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The function find_instance_class only looks for direct assignments in the current file and doesn't handle imports or assignments in other scopes. This means hovering over instance attributes will fail if the instance was created in a different file or imported. Consider documenting this limitation clearly or tracking variable scope more accurately.

Copilot uses AI. Check for mistakes.
Comment on lines +38 to +39
assert result["bounds"] == "(0" # Simple parsing doesn't handle nested parens well
assert result["doc"] == "'A number'"

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The test acknowledges that the parser doesn't handle nested parentheses correctly by checking for the incorrect value '(0' instead of '(0, 100)'. This test is validating broken behavior rather than correct behavior. Either fix the parser to handle nested structures correctly or document this as a known limitation with a TODO comment.

Copilot uses AI. Check for mistakes.
skills=["Python", "Data Science"],
)

print(f"Person: {person.name}, {person.age} years old", person.email)

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

Missing comma after print statement arguments. This will cause a syntax error. Should be print(f"Person: {person.name}, {person.age} years old", person.email)print(f"Person: {person.name}, {person.age} years old, {person.email}")

Copilot uses AI. Check for mistakes.
}

# Regex to find param declarations: name = param.Type(...)
PARAM_PATTERN = re.compile(r"^\s*(\w+)\s*=\s*(param\.(\w+))\s*\(([^)]*)\)", re.MULTILINE)

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The regex pattern PARAM_PATTERN only matches single-line parameter declarations. Multi-line declarations like name = param.String(\n default="value"\n) will not be detected. Consider using the re.DOTALL flag or implementing a more robust parsing strategy.

Copilot uses AI. Check for mistakes.
Comment thread .pre-commit-config.yaml
@@ -58,6 +58,7 @@ repos:
rev: v9.13.0
hooks:

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The added files pattern restricts eslint to only check files in holoviz_mcp/ directory, which means the JavaScript files in examples/param-lsp/vscode-client/ won't be linted. This is inconsistent with having JavaScript code in the repository. Consider either extending the pattern to include example JS files or documenting why they're excluded.

Suggested change
hooks:
hooks:
# NOTE: This eslint hook is intentionally limited to the holoviz_mcp package.
# Example JavaScript/TypeScript files under examples/ (e.g.
# examples/param-lsp/vscode-client/) are not linted by this hook.

Copilot uses AI. Check for mistakes.
Comment on lines +434 to +435
message=f"{type_name} should specify 'objects' parameter",
severity=lsp.DiagnosticSeverity.Warning,

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

The diagnostic for Selector types checks if 'objects' is missing, but according to param documentation and the example_parameterized.py file, Selector can have 'objects' as a keyword argument. However, a Selector can also accept a default value and infer objects from that. The diagnostic might produce false positives. Consider refining this check or downgrading the severity.

Suggested change
message=f"{type_name} should specify 'objects' parameter",
severity=lsp.DiagnosticSeverity.Warning,
message=(
f"Consider specifying 'objects' for {type_name}, or ensure the "
"default value clearly defines the available options"
),
severity=lsp.DiagnosticSeverity.Hint,

Copilot uses AI. Check for mistakes.
Comment on lines +7 to +8
// Path to the Python LSP server - use absolute path
const serverPath = '/home/jovyan/repos/private/holoviz-mcp/examples/param-lsp/param_lsp.py';

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

This hardcoded absolute path will break for any other user. Consider using a relative path from the extension directory or making it configurable through VS Code settings. You could use path.join(__dirname, '..', 'param_lsp.py') to construct a relative path.

Suggested change
// Path to the Python LSP server - use absolute path
const serverPath = '/home/jovyan/repos/private/holoviz-mcp/examples/param-lsp/param_lsp.py';
// Path to the Python LSP server - resolve relative to this extension directory
const serverPath = path.join(__dirname, '..', 'param_lsp.py');

Copilot uses AI. Check for mistakes.
@@ -0,0 +1,46 @@
const path = require('path');

Copilot AI Jan 18, 2026

Copy link

Choose a reason for hiding this comment

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

Unused variable path.

Suggested change
const path = require('path');

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants