Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add notebooks suppport to pylsp #389

Merged
merged 27 commits into from
Aug 9, 2023

Conversation

tkrabel-db
Copy link
Contributor

@tkrabel-db tkrabel-db commented Jun 7, 2023

Jargon

  • LC = Language client
  • LS = Language server

What changes are proposed?

Add Notebook Support to LSP for notebookDocument/didOpen, */didChange, */didClose messages (#308). Other messages are pending.

How is this tested?

I added unit tests and introduced a test helper classes that let us send notification messages to the language server and inspect its state. This helps us assert on messages being sent between LC and LS. More specifically, I tested that the new LS sends correct diagnostics on notebookDocument/didOpen and notebookDocument/didChange, and that the server keeps the workspace in sync. These helpers could also be used to add stronger testing to textDocument related features.

Support notebookDocument/didOpen and textDocument/publishDiagnostics on a notebook document
@tkrabel-db tkrabel-db marked this pull request as draft June 7, 2023 09:07
@tkrabel-db
Copy link
Contributor Author

@krassowski @ccordoba12 FYI. Let's talk code! :)

pylsp/lsp_types.py Outdated Show resolved Hide resolved
Copy link
Contributor

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

My first impression is that this is a good initial implementation. There are many things that can be improved (but could be subsequent PRs).

}

cell_list.append(data)
total_source = total_source + "\n" + cell_document.source
Copy link
Contributor

Choose a reason for hiding this comment

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

(this does not need to happen in this PR) different linters will complain about to few or too many.

  • Maybe number of lines could be configurable.
  • Maybe there should be a way to maks out irrelevant diagnostics on junctions of cells.

Copy link
Contributor

Choose a reason for hiding this comment

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

This and the other JSON file appear unused (in favour of hard-coded expectations).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I was adding them so that future readers have quick examples to get a feeling for potential diagnostics. At least examples would have been very helpful for me. However, I have just a 2/5 preference for keeping examples in the repo, so happy to remove them. WDYT?

test/test_notebook_document.py Outdated Show resolved Hide resolved
pylsp/python_lsp.py Show resolved Hide resolved
pylsp/python_lsp.py Outdated Show resolved Hide resolved
pylsp/python_lsp.py Outdated Show resolved Hide resolved
@krassowski
Copy link
Contributor

Windows failures appear relevant (is this an async testing issue on older Python versions?).

@tkrabel-db
Copy link
Contributor Author

Windows failures appear relevant (is this an async testing issue on older Python versions?).

From the Test logs, it seems that the test cases timed out because loading the server and plugins took too long. I'll increase the timeout to 30 seconds. Let's check if that helps.

@tkrabel-db
Copy link
Contributor Author

tkrabel-db commented Jul 22, 2023

@krassowski can you TAL again? I think I have addressed the important issues. Others will be tracked as follow ups. I also ran pylint and pycodestyle on my code base to make sure we're not running into style issues on CI

…iagnostics notification since the first call is a n empty diagnostics for the closed cell
@tkrabel-db
Copy link
Contributor Author

tkrabel-db commented Jul 24, 2023

@krassowski I fixed a regression to test_notebook_document.py that got introduced by e79bbb2. I ran the test locally to verify it works. Is there a pre-commit hook already in place to make sure pylint, pycodestyle, and pytest run without issues?
Also, do you mind giving me permissions to kick off the workflows myself? This way I can iterate more quickly on the PR.

@ccordoba12
Copy link
Member

Is there a pre-commit hook already in place to make sure pylint, pycodestyle, and pytest run without issues?

Nop, there isn't, sorry.

Also, do you mind giving me permissions to kick off the workflows myself? This way I can iterate more quickly on the PR.

I think that's controlled by Github. You'll be automatically granted permissions once your first PR is merged into this repo.

@tkrabel-db
Copy link
Contributor Author

tkrabel-db commented Jul 25, 2023

Is there a pre-commit hook already in place to make sure pylint, pycodestyle, and pytest run without issues?

Nop, there isn't, sorry.

@ccordoba12 if it's ok, I'd add pre-commit hooks in a separate PR.

@ccordoba12 ccordoba12 added this to the v1.8.0 milestone Jul 29, 2023
@ccordoba12 ccordoba12 changed the title Support the Notebook in LSP Add notebooks suppport to pylsp Jul 29, 2023
Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @tkrabel-db!

README.md Outdated Show resolved Hide resolved
pylsp/python_lsp.py Outdated Show resolved Hide resolved
pylsp/python_lsp.py Outdated Show resolved Hide resolved
pylsp/workspace.py Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

This file has similar code to the one already present in test/test_language_server.py, such as CALL_TIMEOUT, start/start_client, _ClientServer/ClientSever, client_server/client_server_pair and test_initialize.

Why is there so much similar code here? Can't you refactor what's already in test/test_language_server.py and use it here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code is similar but has a substantial difference, namely that in test_notebook_document.py. ClientServer returns also a reference to the server so that we can inspect the server's workspace and e.g. spy on server messages being send as a reaction to client messages. This allows us to fully test client server communication. CALL_TIMEOUT has different values due to the nuances of what we test. I am happy to align that later if I better understand what is exactly happening in test_language_server.py. The tests there are skipped in certain situations and marked as flaky which makes me think that aligigning the two suites will take some deeper thinking and time.
However, I do agree that this should be refactored, and I'd like to do this in a follow up ticket.
WDYT @ccordoba12?

Copy link
Member

Choose a reason for hiding this comment

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

The tests there are skipped in certain situations and marked as flaky which makes me think that aligigning the two suites will take some deeper thinking and time.

Ok, I see.

However, I do agree that this should be refactored, and I'd like to do this in a follow up ticket.

Ok, I saw that you opened issue #410 about it, so that's fine for me.

@tkrabel-db tkrabel-db mentioned this pull request Aug 1, 2023
@ccordoba12 ccordoba12 added the enhancement New feature or request label Aug 9, 2023
@ccordoba12
Copy link
Member

Pinging @krassowski about this one to see if he's ok with it. If I don't hear from him in the next couple of days, I'll merge because it looks good to me now.

@ccordoba12 ccordoba12 mentioned this pull request Aug 9, 2023
Copy link
Contributor

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

No objections from me, looks good for a first step.

Copy link
Member

@ccordoba12 ccordoba12 left a comment

Choose a reason for hiding this comment

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

Thanks @tkrabel-db!

@ccordoba12 ccordoba12 merged commit 2757c3a into python-lsp:develop Aug 9, 2023
10 checks passed
@jasongrout-db
Copy link

Thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants