Implement PythonTA LSP#9
Conversation
b227c95 to
2dede84
Compare
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
@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) | ||
|
|
||
|
|
| # _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 [] | ||
|
|
There was a problem hiding this comment.
Revert this change (keep the blank line)
| content = content[json_start:] | ||
|
|
||
| results = json.loads(content) | ||
| for file_result in results: |
There was a problem hiding this comment.
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.
| # 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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:
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:
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!
|
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 |
david-yz-liu
left a comment
david-yz-liu
left a comment
There was a problem hiding this comment.
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.
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.
2 participants
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
Xor a brief description next to the type or types that best describe your changes.)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:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)