Skip to content

Implement PythonTA LSP#9

Open
a1-su wants to merge 5 commits into
mainfrom
implement-python-ta-lsp
Open

Implement PythonTA LSP#9
a1-su wants to merge 5 commits into
mainfrom
implement-python-ta-lsp

Conversation

@a1-su

@a1-su a1-su commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Proposed Changes

This PR introduces a simple implementation of the PythonTA LSP server, instead of manually running it from a keyboard command. However, it does not include the option to configure your changes yet, so that is a future extension.

Type of Change

(Write an X or a brief description next to the type or types that best describe your changes.)

Type Applies?
🚨 Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change that adds functionality) X
🐛 Bug fix (non-breaking change that fixes an issue)
🎨 User interface change (change to user interface; provide screenshots)
♻️ Refactoring (internal change to codebase, without changing functionality)
🚦 Test update (change that only adds or modifies tests)
📦 Dependency update (change that updates a dependency)
📖 Documentation update (change that updates documentation)
🔧 Internal (change that only affects developers or continuous integration)

Checklist

(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the [ ] into a [x] in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)

Before opening your pull request:

  • I have performed a self-review of my changes.
    • Check that all changed files included in this pull request are intentional changes.
    • Check that all changes are relevant to the purpose of this pull request, as described above.
  • I have added tests for my changes, if applicable.
    • This is required for all bug fixes and new features.
  • I have updated the project documentation, if applicable.
    • This is required for new features.
  • If this is my first contribution, I have added myself to the list of contributors.
  • I have updated the project Changelog (this is required for all changes).

After opening your pull request:

  • I have verified that the pre-commit.ci checks have passed.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported by Coveralls.
  • I have requested a review from a project maintainer.

Questions and Comments

(Include any questions or comments you have regarding your changes.)

@a1-su
a1-su force-pushed the implement-python-ta-lsp branch from b227c95 to 2dede84 Compare July 21, 2026 06:58

@david-yz-liu david-yz-liu 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.

@a1-su nice work, I left a few comments and also please resolve the merge conflicts (from your previous PR).

return uris.to_fs_path(file_uri)
return uris.to_fs_path(document.uri)


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.

Revert this change

# _run_tool_on_document and _run_tool functions as needed for your project.
result = _run_tool_on_document(document)
return _parse_output_using_regex(result.stdout) if result.stdout else []

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.

Revert this change (keep the blank line)

Comment thread bundled/tool/lsp_server.py Outdated
content = content[json_start:]

results = json.loads(content)
for file_result in results:

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.

Overall I think this code can be simplified using lsp.converters.get_converter(). This returns a cattrs converter that should be usable to parse the JSON into the relevant lsp classes, since the PythonTA reporter should be set up using them already.

Comment thread bundled/tool/lsp_server.py Outdated
# Pass document so get_cwd can resolve file-related variables for this document.
cwd = get_cwd(settings, document)

if settings["interpreter"] and len(settings["interpreter"]) > 0:

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.

I'm not sure why you changed the logic in this part of the code from what the template already provides. Let's just stick with the template here, we can always extend it later if we want.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I completely agree we should stick with what the template provides, but I believe for PythonTA to function correctly, adding this block of code might be necessary? For reference, here is the fatal error from PythonTA that we get when we run the extension without these lines:
Image

And the file is also missing all the PythonTA-specific errors that we would see if we were to run PythonTA on the code normally. These are the only errors we see when we run the extension without this block of code:
Image

And these are all the errors we should be seeing (matches what we would see in an HTML output) when PythonTA is run with this block of code included:
Image

I could be wrong, but when I ran into this issue before, it was astroid using the environment of the extension (language server specifically, in this case) itself, instead of the user's workspace, so we prepend the proper virtual environment directory to the PATH to use that instead. If there's a better way, feel free to let me know!

@a1-su

a1-su commented Jul 23, 2026

Copy link
Copy Markdown
Contributor Author

As a side note, I've essentially reverted the changes I made initially in the first PR (#8) since it was a proof of concept, and extension.ts doesn't need to be / shouldn't be changed from its original version for the LSP server implementation to work.

@a1-su
a1-su requested a review from david-yz-liu July 23, 2026 03:37

@david-yz-liu david-yz-liu 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.

Hi @a1-su, I'll respond to your comment after testing this out locally.

I ran into an issue when running the extension: looks like an EOFError was raised.

Image

I'm not sure what the best way to debug this is, but please first update the README with more detailed information on how you are starting and running the extension.

@a1-su

a1-su commented Jul 26, 2026

Copy link
Copy Markdown
Contributor Author

Hi @a1-su, I'll respond to your comment after testing this out locally.

I ran into an issue when running the extension: looks like an EOFError was raised.
Image

I'm not sure what the best way to debug this is, but please first update the README with more detailed information on how you are starting and running the extension.

As I was investigating the issue, I think I realized what the bug was and removed the code segment. If I had to guess, the EOFError you're encountering might be because we need to include some dependencies in bundled/lib as well, and I didn't include the proper instructions initially. In particular, we need to use a local version of PythonTA, since the latest release doesn't include the pyta-lsp option for the output format that we need for the extension. I've updated the README with the instructions as well.

The actual error was in the PythonTA repository. In the static_type_checker.py calls mypy directly, which uses the OS's PATH to determine which mypy executable this is.

https://github.com/pyta-uoft/pyta/blob/0e32f5244333e3cc3f27a2c1147aa93398bee8be/packages/python-ta/src/python_ta/checkers/static_type_checker.py#L91

call = ["mypy", filename] + mypy_options

However, if we use sys.executable, we can execute mypy using the selected Python interpreter. This isn't typically a problem with how PythonTA usually runs, but it means for the extension, we can have the language server run the proper mypy without explicitly updating the path with the Python interpreter.

call = [sys.executable, "-m", "mypy", filename] + mypy_options

I've made a PR on the PythonTA repository with this change, so if that change gets made and you install the local version with change, hopefully the extension will work properly 👍

Comment thread README.md

1. Install Python dependencies: `uv sync`.
2. Install Javascript dependencies: `pnpm install`.
3. Clone the [`pyta`](https://github.com/pyta-uoft/pyta) repository locally. The most recent release does not include the `pyta-lsp` output format needed for the VSCode extension.

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.

This isn't necessary. Modify the pyproject.toml to specify the GitHub URL for installing PythonTA.

For the bundled/libs issue, I think you can modify step 1 above to replace the uv sync with a uv pip install --target bundled/libs -r pyproject.toml

Comment thread README.md
```

To start the extension, use the `Debug Extension and Python` configuration in VS Code.
To start the extension, use the `Debug Extension and Python` (shortcut `F5`) configuration in VS Code.

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.

So overall I'm able to start the extension now and I don't think I see any errors, but I don't see anything happening, either. Please provide instructions in the README on what to do after the extension window opens. Add a video to the PR description as well.

# 'path' setting takes priority over everything.
use_path = True
argv = settings["path"]
elif settings["interpreter"] and not utils.is_current_interpreter(

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.

I see you deleted some of the code you added, but it also seems like we should then restore this block here.

@david-yz-liu

Copy link
Copy Markdown
Contributor

@a1-su Claude helped identify an issue with the document URI filtering (Line 256 of lsp_server.py). There seems to be a path mismatch on Windows, and it suggest defining a normalization function:

def _normalize_uri_path(uri: str) -> str:
    """Normalizes a file URI to a comparable filesystem path.

    On Windows, `pathlib.Path.resolve()` uppercases drive letters (used by
    PythonTA's JSON reporter), while VS Code always sends `document.uri` with
    a lowercase drive letter. Comparing raw URI strings therefore never
    matches on Windows, so diagnostics must be compared as normalized paths.
    """
    return os.path.normcase(os.path.normpath(uris.to_fs_path(uri)))

Can you investigate this as well?

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