ruff icon indicating copy to clipboard operation
ruff copied to clipboard

`ruff server`: Support Jupyter Notebook (`*.ipynb`) files

Open snowsignal opened this issue 9 months ago • 4 comments

Summary

Closes https://github.com/astral-sh/ruff/issues/10858.

ruff server now supports *.ipynb (aka Jupyter Notebook) files. Extensive internal changes have been made to facilitate this, which I've done some work to contextualize with documentation and an pre-review that highlights notable sections of the code.

*.ipynb cells should behave similarly to *.py documents, with one major exception. The format command ruff.applyFormat will only apply to the currently selected notebook cell - if you want to format an entire notebook document, use Format Notebook from the VS Code context menu.

Test Plan

The VS Code extension does not yet have Jupyter Notebook support enabled, so you'll first need to enable it manually. To do this, checkout the pre-release branch and modify src/common/server.ts as follows:

Before: Screenshot 2024-05-13 at 10 59 06 PM

After: Screenshot 2024-05-13 at 10 58 24 PM

I recommend testing this PR with large, complicated notebook files. I used notebook files from this popular repository in my preliminary testing.

The main thing to test is ensuring that notebook cells behave the same as Python documents, besides the aforementioned issue with ruff.applyFormat. You should also test adding and deleting cells (in particular, deleting all the code cells and ensure that doesn't break anything), changing the kind of a cell (i.e. from markup -> code or vice versa), and creating a new notebook file from scratch. Finally, you should also test that source actions work as expected (and across the entire notebook).

Note: ruff.applyAutofix and ruff.applyOrganizeImports are currently broken for notebook files, and I suspect it has something to do with https://github.com/astral-sh/ruff/issues/11248. Once this is fixed, I will update the test plan accordingly.

snowsignal avatar Apr 29 '24 17:04 snowsignal

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

github-actions[bot] avatar Apr 29 '24 18:04 github-actions[bot]

I think there's another problem here. To demonstrate that refer:

ruff-lsp

https://github.com/astral-sh/ruff/assets/67177269/ea494142-a2dc-4d59-b0db-98a28589f480

ruff server

https://github.com/astral-sh/ruff/assets/67177269/0c6c2473-6bab-47ce-80d8-00d3b2588556

You can see that there's an additional newline which gets added at the end for ruff server. The reason this is happening is because the cell content are concatenated by a newline but the newline doesn't actually exists in the original source code.

The reason it doesn't happen in ruff-lsp is because we post-process the Notebook where we strip this additional newline before updating the cell content:

https://github.com/astral-sh/ruff/blob/75f302298c2b6d481a466c463389ce770aa38f6b/crates/ruff_notebook/src/notebook.rs#L243-L273

This post-processing doesn't happen for the new server because it uses a wrapper to lint / format (lint.rs and format.rs).

dhruvmanila avatar May 16 '24 05:05 dhruvmanila

Notes from testing

  1. Did you implement the notebook.* variants of the source code actions? It doesn't seem to be working. They are notebook.source.fixAll and notebook.source.organizeImports which are specific to Notebook. Here, the server will receive only the first cell and we're responsible on returning the edits for the entire notebook. I'm using the following VS Code settings:

      "notebook.codeActionsOnSave": {
        "notebook.source.fixAll": "explicit"
      }
    

    For reference, https://github.com/astral-sh/ruff-lsp/blob/b8a48009f35756dc51268beb6bbc39c21a1e3a46/ruff_lsp/server.py#L219-L229

  2. This might be unrelated to this PR but I'm getting the following error quite frequently:

    2024-05-16 17:34:08.101 [info]  196.046579s WARN ruff_server::server::api Received notification $/setTrace which does not have a handler.
    
    2024-05-16 17:34:10.471 [info]  198.416243s ERROR ruff_server::server::api An error occurred while running textDocument/didClose: Unable to take snapshot for document with URL file:///Users/dhruv/.pyenv/versions/3.7.17/lib/python3.7/random.py
    

    It's usually occurs after the setTrace notification. The confusing part is that I have no idea why is it occurring on textDocument/didClose when I haven't closed any document.

  3. Based on this in the PR description:

    Note: ruff.applyAutofix and ruff.applyOrganizeImports are currently broken for notebook files, and I suspect it has something to do with #11248. Once this is fixed, I will update the test plan accordingly.

    Were you able to look into this and fix it? I'm unable to run any "Ruff:" prefixed commands except for "Format Document".

dhruvmanila avatar May 16 '24 12:05 dhruvmanila

@dhruvmanila

Did you implement the notebook.* variants of the source code actions?

No, this still needs to be added.

This might be unrelated to this PR but I'm getting the following error quite frequently

This is an unrelated bug that I've seen several times before. I'll open a proper issue for it.

Were you able to look into this and fix it? I'm unable to run any "Ruff:" prefixed commands except for "Format Document".

No, this still needs to be fixed.

snowsignal avatar May 16 '24 15:05 snowsignal

You might want to rebase to make the CI green :)

dhruvmanila avatar May 21 '24 15:05 dhruvmanila