Add notebooks suppport to pylsp by tkrabel-db · Pull Request #389 · python-lsp/python-lsp-server

tkrabel-db · 2023-06-07T09:07:31Z

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 · 2023-06-07T09:09:05Z

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

pylsp/lsp_types.py

too-many-public-methods, dangerous-default-value, unused-import, too-many-locals, redefined-outer-name

krassowski

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

krassowski · 2023-06-27T16:29:11Z

pylsp/python_lsp.py

+            }
+
+            cell_list.append(data)
+            total_source = total_source + "\n" + cell_document.source


(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.

krassowski · 2023-06-27T16:54:37Z

test/data/publish_diagnostics_message_examples/example_1.json

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

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

pylsp/python_lsp.py

krassowski · 2023-06-27T18:00:03Z

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

tkrabel-db · 2023-07-11T12:25:04Z

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 · 2023-07-22T11:49:33Z

@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 · 2023-07-24T21:14:14Z

@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 · 2023-07-24T21:19:50Z

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 · 2023-07-25T07:12:11Z

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

Thanks for your contribution @tkrabel-db!

README.md

pylsp/python_lsp.py

pylsp/workspace.py

test/data/publish_diagnostics_message_examples/example_1.json

ccordoba12 · 2023-07-29T05:04:29Z

test/test_notebook_document.py

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?

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?

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.

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

ccordoba12 · 2023-08-09T16:38:05Z

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.

krassowski

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

ccordoba12

Thanks @tkrabel-db!

jasongrout-db · 2023-08-10T23:21:54Z

Thanks for the reviews!

tkrabel-db added 2 commits June 5, 2023 12:21

README.md

70076fa

Add notebook support

ba0f9ad

Support notebookDocument/didOpen and textDocument/publishDiagnostics on a notebook document

tkrabel-db marked this pull request as draft June 7, 2023 09:07

tkrabel-db commented Jun 7, 2023

View reviewed changes

pylsp/lsp_types.py Outdated Show resolved Hide resolved

tkrabel-db mentioned this pull request Jun 7, 2023

Adding support NotebookDocumentClientCapabilities #308

Open

tkrabel-db added 9 commits June 7, 2023 11:19

Add example messages for reference

d5bf4f9

lsp_types: add DocumentUri and use correctly

78233c7

add diagnostics support

09d6879

support editing, adding, and removing cells

500e120

add unit tests for notebook document messages

b407a31

add test_notebook_document

53e4ad6

fix unit tests and pylint

c891cf0

fix pytest test_notebook_document.py

3dcaf9f

Fix pylint issues:

ab3bcee

too-many-public-methods, dangerous-default-value, unused-import, too-many-locals, redefined-outer-name

tkrabel-db marked this pull request as ready for review June 14, 2023 15:48

jtpio mentioned this pull request Jun 14, 2023

Weekly Team Meetings: Jan-Jun 2023 jupyterlab/frontends-team-compass#170

Closed

tkrabel-db added 3 commits June 14, 2023 21:00

support notebookDocument__did_close

2dd7165

Add notebookDocumentSync to capabilities

7722239

Add notebookDocumentSync to capabilities

6c00bfc

krassowski reviewed Jun 27, 2023

View reviewed changes

tkrabel-db added 3 commits July 10, 2023 15:10

fix: publishDiagnostics line starts at line 0

60517c1

fix: publishDiagnostics starts at 0 and newlines are counted correctly

ecc27ee

fix: publishDiagnostics starts at 0 and newlines are counted correctly

25cdfab

tkrabel-db added 4 commits July 11, 2023 14:27

fix: publishDiagnostics starts at 0 and newlines are counted correctly

2b35c95

fix: publishDiagnostics starts at 0 and newlines are counted correctly

69a1621

fix: publishDiagnostics starts at 0 and newlines are counted correctly

1e9a51f

fix: publishDiagnostics starts at 0 and newlines are counted correctly

4f140da

tkrabel-db requested a review from krassowski July 11, 2023 14:18

tkrabel-db added 2 commits July 20, 2023 13:31

feat: close cell if deleted

e79bbb2

skip tests on windows as it's flaky on py3.7

ce9f458

fix test_notebook_document__did_change: need to wait for 2 calls to d…

5434d1f

…iagnostics notification since the first call is a n empty diagnostics for the closed cell

jasongrout mentioned this pull request Jul 28, 2023

Notebook protocol go-to-definition support #408

Merged

ccordoba12 added this to the v1.8.0 milestone Jul 29, 2023

ccordoba12 changed the title ~~Support the Notebook in LSP~~ Add notebooks suppport to pylsp Jul 29, 2023

ccordoba12 reviewed Jul 29, 2023

View reviewed changes

tkrabel-db and others added 3 commits July 29, 2023 21:03

Update test/data/publish_diagnostics_message_examples/example_1.json

ceb193d

Co-authored-by: Carlos Cordoba <ccordoba12@gmail.com>

remove aliases

e7df839

address comments

2dd05b7

tkrabel-db requested a review from ccordoba12 August 1, 2023 16:42

tkrabel-db mentioned this pull request Aug 1, 2023

Clean up unit tests #410

Closed

ccordoba12 added the enhancement New feature or request label Aug 9, 2023

ccordoba12 mentioned this pull request Aug 9, 2023

Black format Repo #407

Closed

krassowski approved these changes Aug 9, 2023

View reviewed changes

ccordoba12 approved these changes Aug 9, 2023

View reviewed changes

ccordoba12 merged commit 2757c3a into python-lsp:develop Aug 9, 2023

Conversation

tkrabel-db commented Jun 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jargon

What changes are proposed?

How is this tested?

Uh oh!

tkrabel-db commented Jun 7, 2023

Uh oh!

Uh oh!

krassowski left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krassowski Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

krassowski Jun 27, 2023

Choose a reason for hiding this comment

Uh oh!

tkrabel-db Jul 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

krassowski commented Jun 27, 2023

Uh oh!

tkrabel-db commented Jul 11, 2023

Uh oh!

tkrabel-db commented Jul 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tkrabel-db commented Jul 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccordoba12 commented Jul 24, 2023

Uh oh!

tkrabel-db commented Jul 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ccordoba12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ccordoba12 Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

tkrabel-db Jul 29, 2023

Choose a reason for hiding this comment

Uh oh!

ccordoba12 Aug 9, 2023

Choose a reason for hiding this comment

Uh oh!

ccordoba12 commented Aug 9, 2023

Uh oh!

krassowski left a comment

Choose a reason for hiding this comment

Uh oh!

ccordoba12 left a comment

Choose a reason for hiding this comment

Uh oh!

jasongrout-db commented Aug 10, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tkrabel-db commented Jun 7, 2023 •

edited

Loading

krassowski left a comment •

edited

Loading

tkrabel-db commented Jul 22, 2023 •

edited

Loading

tkrabel-db commented Jul 24, 2023 •

edited

Loading

tkrabel-db commented Jul 25, 2023 •

edited

Loading