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

Initial support for notebook document sync methods #356

Merged
merged 12 commits into from
Sep 19, 2023

Conversation

alcarney
Copy link
Collaborator

@alcarney alcarney commented Aug 6, 2023

Description (e.g. "Related to ...", etc.)

nbsync

This is a first pass at implementing support for the notebookDocument/did* group of messages from the LSP spec.

  • This PR depends on a future version of lsprotocol that has some additional fixes
    (see Missing structure hook(s?) for notebook types  microsoft/lsprotocol#228, Unable to structure NotebookDocumentSyncOptions microsoft/lsprotocol#259)

  • Servers can advertise support for notebooks by passing an instance of NotebookDocumentSyncOptions to the LanguageServer constructor. e.g. from the inlay_hints.py server

    server = LanguageServer(
        name="inlay-hint-server",
        version="v0.1",
        notebook_document_sync=types.NotebookDocumentSyncOptions(
            notebook_selector=[
                types.NotebookDocumentSyncOptionsNotebookSelectorType2(
                    cells=[
                        types.NotebookDocumentSyncOptionsNotebookSelectorType2CellsType(
                            language="python"
                        )
                    ]
                )
            ]
        ),
    )
  • Notebooks as a whole are represented using instances of the NotebookDocument class that comes from lsprotocol

  • Like regular text documents, notebook cell contents are represented as instances of pygls.workspace.TextDocument (previously called Document)

  • Notebooks can be accessed from the Workspace using the new workspace.get_notebook_document() method either

    • by passing the notebook_uri of the notebook itself or
    • by passing cell_uri of one of the text documents representing the contents of a cell in the notebook
  • Currently this is the only notebook specific method exposed to server authors - @karthiknadig let me know if you have anything specific you'd like pygls to handle/provide

  • Generic references to "document" have been renamed to "text_document" so that it's clear which kind of document a method/object is referring to. Old names still work, but emit deprecation warnings where possible - see 9034bdb

  • The example inlays_hints.py server has been updated to support notebooks.
    # characters on a line on their own are annotated with the notebook's uri and index of the cell it resides in - let me know if you can think of a more interesting demonstration of the notebook support! 😅

Closes #311

Code review checklist (for code reviewer to complete)

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Commit messages are meaningful (see this for details)
  • Tests have been included and/or updated, as appropriate
  • Docstrings have been included and/or updated, as appropriate
  • Standalone docs have been updated accordingly

@alcarney alcarney mentioned this pull request Aug 6, 2023
6 tasks
@alcarney alcarney force-pushed the notebook-document-sync branch 2 times, most recently from 28dbeba to a6e6851 Compare September 8, 2023 13:02
@alcarney alcarney marked this pull request as ready for review September 8, 2023 14:30
@alcarney alcarney requested a review from tombh September 8, 2023 14:30
@alcarney
Copy link
Collaborator Author

alcarney commented Sep 8, 2023

This is probably good enough to merge now, I expect the best way to get some feedback is to get this out there in a release so people can try it out

Any thoughts?

pygls/server.py Outdated
converter_factory: Callable[[], cattrs.Converter],
loop: Optional[asyncio.AbstractEventLoop] = None,
max_workers: int = 2,
sync_kind: Optional[TextDocumentSyncKind] = None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this remain Incremental?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this will break servers. Many of the servers I have written assume this is set to Incremental by default.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should've been ok, since this is the base Server class where I don't think this was really used.
It's also set by the LanguageServer constructor.

That said, I don't think we gain anything from changing this so I've rolled it back anyway 😄

@alcarney alcarney force-pushed the notebook-document-sync branch 2 times, most recently from 3615dd7 to 12c63c2 Compare September 9, 2023 09:55
Now that there is a distinction between text documents and notebook
documents, this commit renames the methods which act on text documents
accordingly.

The previous method names will still work, but now emit deprecation
warnings
In order to enable `notebookDocument/did*` messages from the client, a
server must provide its chosen `NotebookDocumentSyncOptions` as part
of its `ServerCapabilities`.

This commit adds a new `notebook_document_sync` option to the
`LanguageServer` constructor allowing this option to be set.
To stop type checkers like pyright complaining about the possibility
of `self.workspace` being `None`, the workspace is now stored in
`self._workspace` and accessed by via a property.

This property performs the check for `None` and raises a RuntimeError
accordingly
@alcarney alcarney force-pushed the notebook-document-sync branch from 12c63c2 to 4774f52 Compare September 10, 2023 21:21
@tombh
Copy link
Collaborator

tombh commented Sep 14, 2023

Let's merge this then? Would be good to get the confirmation from Karthik, but sounds like you're happy Alex?

@alcarney
Copy link
Collaborator Author

but sounds like you're happy Alex?

Yes :)

@tombh tombh merged commit 9e784e3 into openlawlibrary:main Sep 19, 2023
17 checks passed
@alcarney alcarney deleted the notebook-document-sync branch September 19, 2023 21:22
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.

Add support for notebooks
3 participants