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.

Sign up for GitHub

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

Jump to bottom

Formatting and "Fix All" doesn't work if it removes entire file content #267

Closed
dhruvmanila opened this issue Oct 7, 2023 · 2 comments · Fixed by #264 or #319
Closed

Formatting and "Fix All" doesn't work if it removes entire file content #267

dhruvmanila opened this issue Oct 7, 2023 · 2 comments · Fixed by #264 or #319
Labels
bug Something isn't working

Comments

@dhruvmanila
Copy link
Member

If you take the following code example in VSCode:

import os

And run the command Ruff: Fix all auto-fixable problems, nothing will happen.

But, suppose you add an additional statement:

import os

print("hello world")

Now, if you run the same command, it'll remove the import.

The reason it's not working in the first case is because we create a WorkspaceEdit only if the fixed source code isn't empty. But, in the first case it's going to be empty.

A new statement isn't even needed, just add some extra newlines and you'll be able to reproduce the bug.
@dhruvmanila dhruvmanila added the bug Something isn't working label Oct 7, 2023
@dhruvmanila dhruvmanila mentioned this issue Oct 23, 2023
Merged
17 tasks
dhruvmanila added a commit that referenced this issue Nov 8, 2023
@dhruvmanila
Add support for Jupyter Notebook (#264)
31f3836 
## Summary

This PR adds support for Jupyter Notebook. It requires client support
for LSP 3.17 which contains the [Notebook
support](https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#notebookDocument_synchronization).

### Implementation

#### Context

* `Document`: LSP type representing a text file (Python file for Ruff).
* `TextDocument`: `pygls` representation of the LSP `Document`. This is
an abstraction created from a `Document` which provides some useful
methods like getting the file path, source code, etc.
* New in 3.17: `NotebookDocument` type was added to represent a Notebook
which consists of a list of cells (`NotebookCell`). Note that these are
all LSP types coming from `lsprotocol`.
* In `pygls`, a Notebook cell is represented as a text document
(`TextDocument`).

There are methods provided by `pygls` to get the object:
* `get_text_document` - Returns a `TextDocument` which either represents
a Python file or a Notebook cell
* `get_notebook_document` - Returns a `NotebookDocument` either using
the Notebook URI or a cell URI. For cell URI, it returns the
`NotebookDocument` containing the cell.

#### Document

A new `Document` type was created to facilitate the implementation. This
represents either a Python file, a Notebook or a Notebook cell. There
are various constructor methods which should be used to create this
type:
* For a URI representing a Python file, use either `from_uri` or
`from_text_document`.
* For a URI representing a Notebook file, use either `from_uri` or
`from_notebook_document`.
* For a URI representing a Notebook cell, use either
`from_cell_or_text_uri` or `from_notebook_cell`.

#### Notebook JSON

Ruff expects the source content of a Notebook file to be in JSON format
following the [Notebook format specification] but the protocol uses it's
own abstraction and doesn't store the JSON format. This means that we
need to create a JSON string representing the Notebook from the
available information. This doesn't need all the information as Ruff
only uses the cell source and version information. So, we create a
minimal JSON string representing the Notebook document and pass it to
Ruff.

<details><summary>An example JSON string representing a Notebook
Document:</summary>
<p>

```json
{
  "metadata": {},
  "nbformat": 4,
  "nbformat_minor": 5,
  "cells": [
    {
      "cell_type": "code",
      "metadata": null,
      "outputs": [],
      "source": "import random\nimport math"
    },
    {
      "cell_type": "code",
      "metadata": null,
      "outputs": [],
      "source": "try:\n    print()\nexcept ValueError:\n    pass"
    },
    {
      "cell_type": "code",
      "metadata": null,
      "outputs": [],
      "source": "import random\nimport pprint\n\nrandom.randint(10, 20)"
    },
    {
      "cell_type": "code",
      "metadata": null,
      "outputs": [],
      "source": "foo = 1\nif foo is 1:\n    msg = f\"Invalid foo: {foo}\"\n    raise ValueError(msg)"
    }
  ]
}
```

</p>
</details> 

**We need to pass in every cell including the markdown cell to get an
accurate information like the cell number.**

For the cell document kind, the source value is a JSON string containing
just a single code cell. This is required as code actions and formatting
work at both cell and notebook level.

### Configuration

For VSCode users, the `notebook.*` configuration is used to run the
formatter or code actions on save:

```jsonc
{
  // Enable formatting the entire Notebook on save
  "notebook.formatOnSave.enabled": true,
  // Run the enabled code actions on the entire Notebook on save
  "notebook.codeActionsOnSave": {
    "source.fixAll": true,
    "source.organizeImports.ruff": true
  },
}
```

The way the above settings work in VSCode is that the editor runs the
actions in parallel for every cell. This has the illusion that it was
run on the entire Notebook. The commands defined by us (`Ruff: Organize
imports` and `Ruff: Fix all auto-fixable problems`) are run on the
entire Notebook at once. This is important because in the latter case
the `ruff` command is invoked `n` number of times where `n` is the
number of cells while for the former it's run only once.

### Commands

#### Builtin

* `Ruff: Organize Imports`: Works at Notebook level
* `Ruff: Fix all auto-fixable problems`: Works at Notebook level

#### VSCode specifc

* `Format Cell`: Formats the current cell
* `Notebook: Format Notebook`: Formats the entire Notebook by running
the formatter for every cell
* `Organize Imports`: Runs the `source.organizeImports` code action on
every cell in parallel
* `Fix All`: Runs the `source.fixAll` code action on every cell in
parallel

## Feature checklist

- [x] Code actions
  - [x] Organize imports
  - [x] Fix all
  - [x] Each fixable diagnostics
  - [x] Disable rule comment
- [x] Code action resolve
- [x] Commands
  - [x] `ruff.applyAutofix`
  - [x] `ruff.applyOrganizeImports`
  - [x] `ruff.applyFormat`
- [x] Diagnostics
  - [x] On open
  - [x] On close
  - [x] On save
  - [x] On change
- [x] Formatting
- [x] Hover

## Test Plan

Manually testing for all the features mentioned above.

### How to run this locally?

1. Clone https://github.com/astral-sh/ruff-lsp and
https://github.com/astral-sh/ruff-vscode in the same directory
2. Checkout this branch `git checkout dhruv/notebook` in the `ruff-lsp`
repository
3. Install the requirements for both repositories
4. For `ruff-vscode`, uninstall `ruff-lsp` (`pip uninstall --yes
ruff-lsp`) as we'd want to use the local version. To install the local
`ruff-lsp` version in `ruff-vscode`, follow [Modifying the
LSP](https://github.com/astral-sh/ruff-vscode#modifying-the-lsp).
5. Open VSCode from `ruff-vscode` directory -> "Run and Debug" section
from the sidebar -> "Debug Extension and Python" config.

This will then open a VSCode development session which can be used to
test out the notebook features.

**Test notebooks:**
* Formatting:
https://gist.github.com/dhruvmanila/7803e5a3b98c414505384db415a635a0
* Diagnostics, Code actions, Commands:
https://gist.github.com/dhruvmanila/54c65870f167a56558d4701f57f53042

**Requires: astral-sh/ruff#7664 which was
released in `v0.1.0`**

fixes: #267 
closes: astral-sh/ruff-vscode#256
closes: astral-sh/ruff-vscode#314
closes: astral-sh/ruff-vscode#51
@dhruvmanila
Copy link
Member Author

(This change was reverted in #317)

@dhruvmanila dhruvmanila reopened this Nov 10, 2023
@dhruvmanila
Copy link
Member Author

Using astral-sh/ruff#8596, we can now safely write empty fixes on supported Ruff version.

@dhruvmanila dhruvmanila changed the title Command ruff.applyAutofix doesn't work if it removes the entire file content Formatting and "Fix All" doesn't work if it removes the entire file content Nov 10, 2023
@dhruvmanila dhruvmanila changed the title Formatting and "Fix All" doesn't work if it removes the entire file content Formatting and "Fix All" doesn't work if it removes entire file content Nov 10, 2023
@charliermarsh charliermarsh mentioned this issue Nov 10, 2023
Merged
charliermarsh added a commit that referenced this issue Nov 10, 2023
@charliermarsh
Always write empty fixes on Ruff >= 0.1.6 (#319)
0edc51e 
## Summary

We can avoid the safeguard we introduced in
#317 for newer Ruff versions,
since Ruff will now avoid writing empty output for excluded files (see:
astral-sh/ruff#8596).

Closes #267.

## Test Plan

I tested this with a local debug build. Now, when I "Fix all" on a
non-excluded file with `import os`, it _does_ properly get removed,
while running "Format" or "Fix all" on an excluded file leaves the
contents untouched.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
1 participant
@dhruvmanila