astral-sh / ruff-vscode

A Visual Studio Code extension with support for the Ruff linter.
Other
946 stars 45 forks source link

Pre-release for new `ruff server`-based extension #431

Closed snowsignal closed 3 months ago

snowsignal commented 3 months ago

Summary

This is a pre-release edition of Ruff for VS Code that uses our new, Rust-based LSP - ruff server.

At the moment, the pre-release has the following known limitations:

These limitations will be addressed in future versions.

To use the new Rust-based language server, you'll need to enable the "Experimental Server" setting and reload the extension. The setting can be disabled again if you encounter any major issues with the new server.

Test Plan

To test the features of the new extension, please do the following:

  1. Enable the "Experimental Server" setting on the extension settings page and restart VS Code.

Testing the Linter

  1. Open a file in your repository of choice and write Python code that would normally cause a lint error in Ruff. For example, this code would cause three lint errors with the default rule selectors:
    
    def useless_semicolon():
    pass;

def multiple_fixes(): x = True if x == False: print("this can't be good")


3. The lint errors should be highlighted in the editor view, and hovering over the highlighted text should display the correct diagnostic.
4. Select a portion of highlighted text and pull up a list of Quick Fixes with `Ctrl/Cmd + .`, or whatever shortcut you use for Quick Fixes. The appropriate fix should appear in the list.
5. Select the fix. The file should be applied correctly.
6. Undo the fix with `Ctrl/Cmd+Z`.
7. Select the entire file with `Ctrl/Cmd+A`.
8. Open up the list of quick fixes for the file with `Ctrl/Cmd + .`. All the lint fixes should be in the dropdown list.
9. Apply each fix to the file, re-opening the list as necessary.
10. Confirm that the file passes the linter by running `ruff check <path to file>` from the workspace root folder.

### Testing the Formatter

11. Add miscellaneous whitespace throughout the file, such that it's incorrectly formatted in multiple places.
12. Highlight a section of incorrectly formatted code and run `Format Selection` from the right-click context menu or with the shortcut `Ctrl/Cmd+K+F`.
13. Confirm that the non-selected code was not edited, and that the currently selected code is now correctly formatted.
14. Undo this range format and test `Format Selection` by highlighting the entire file with `Ctrl/Cmd+A`.
16. Confirm that the file is formatted correctly by running `ruff format --check <path to file>` from the workspace root folder.
17. Undo the format change.
18. Right click and select `Format Document` from the VS Code context menu, or use the shortcut `Alt+Shift+F`.
19. Confirm that the file is formatted correctly by running `ruff format --check <path to file>` from the workspace root folder.

### Testing the Configuration Reloader
20. To test that your workspace configuration reloads, open the `ruff.toml`/`pyproject.toml` configuration file at your workspace root (if one exists). If it does not exist, create one.
21. Decide on a rule to test (ideally one that is not already selected in the config)
22. Open a Python file within the workspace and write code that would violate this rule. Confirm that it does not raise a lint error.
23. Open the config file and add this rule in `select`.
24. At the moment, diagnostics can be a bit stale after a configuration reload (this will be fixed in a future release). To get around this, edit a single character of the offending code to 'refresh' it.
25. The lint error for the rule should now appear on the offending code.
26. Remove the rule from `select` in the config.
27. 'Refresh' the offending code, and confirm that the lint error no longer appears.
charliermarsh commented 3 months ago

Sweet! Before I review the code itself, a few questions on the summary:

For example, lint.args / format.args will be replaced in the future with specific configuration fields for the linter and formatter.

What does this mean in practice? Will there be a separate setting in VS Code for every configuration option in the linter and formatter?

Commands like Fix all and Quick Fix have not yet been implemented. (code actions should still work, though)

What does it mean for Code Actions to work, but those commands to not work? What is an example of a Code Action that does work?

charliermarsh commented 3 months ago

I'm still looking to understand the existing limitations of the LSP. I think it should meet some basic tablestakes functionality before we advertise it in any way, and parts of this PR feel like they are advertising it.

dhruvmanila commented 3 months ago

A few thoughts running this locally:

  1. I don't think we should provide an error stating when the server isn't able to find the configuration file. This probably needs to be handled inside ruff.
2024-03-22 00:28:04.830 [info]    0.301909s ERROR ruff_server::session The following error occurred when trying to find a configuration file at `/private/tmp/ruff-vscode`:
No pyproject.toml/ruff.toml/.ruff.toml file was found
   0.301931s ERROR ruff_server::session Falling back to default configuration for `/private/tmp/ruff-vscode`
  1. There are few log entries which doesn't seem relevant. You might want to look at where they are originating from and probably remove it. Referring to the notifications::did_open and notifications::cancel entries in the following. I think these are tracing logs which are being emitted.
2024-03-22 00:33:53.544 [info] ┐ruff_server::server::api::notifications::did_open::run{file=file:///Users/dhruv/playground/ruff/formatter/main.py}
┘

2024-03-22 00:33:53.559 [info] [Trace - 12:33:53 AM] Received response 'textDocument/diagnostic - (1)' in 171ms.
2024-03-22 00:33:53.561 [info] [Trace - 12:33:53 AM] Sending notification '$/cancelRequest'.
2024-03-22 00:33:53.561 [info] [Trace - 12:33:53 AM] Sending request 'textDocument/codeAction - (3)'.
2024-03-22 00:33:53.565 [info] [Trace - 12:33:53 AM] Received response 'textDocument/codeAction - (2)' in 130ms.
2024-03-22 00:33:53.566 [info] ┐ruff_server::server::api::notifications::cancel::run{}
┘

2024-03-22 00:33:53.614 [info] [Trace - 12:33:53 AM] Received response 'textDocument/codeAction - (3)' in 53ms.
  1. This is just a suggestion and not really required to be implemented but as the commands aren't implemented, maybe we could provide a info level log when a user tries to execute them. Currently, there's a log stating that there's no handler for the command in the server which users might find confusing.

  2. We might want to clarify that we don't support source level code actions yet which means the editor.codeActionsOnSave config wouldn't work.

  3. I added the activation events for Jupyter Notebook back in package.json and it's fine to do so as they will be ignored by the document selector setting. Confirmed that Notebooks aren't being picked up in that case.

snowsignal commented 3 months ago

@charliermarsh Thanks for your feedback, I just had a few questions:

I'm still looking to understand the existing limitations of the LSP.

I've tried to cover the limitations in the new section of the README.md and in the MR description. Is there a different way I should be communicating these limitations? Is the list of limitations vague or missing items? Or is it something else?

I think it should meet some basic tablestakes functionality before we advertise it in any way, and parts of this PR feel like they are advertising it.

What do you think is missing from the LSP right now that should be table-stakes functionality?

charliermarsh commented 3 months ago

@snowsignal - I mostly didn't understand this part, I asked about it above:

Commands like Fix all and Quick Fix have not yet been implemented. (code actions should still work, though)

What does it mean for Code Actions to work, but those commands to not work? What is an example of a Code Action that does work?

What do you think is missing from the LSP right now that should be table-stakes functionality?

I was hoping that we'd support Fix All, Organize Imports, and Format. (Non-table-stakes would be like: Jupyter support, support for automatically adding the # noqa suppressions as code actions, etc.). I'm okay with releasing without those capabilities if we want to test it and get into a shipping motion, I just don't know if we should publicize it in the README etc., since those are required for ~any user to use the LSP productively, I think.

snowsignal commented 3 months ago

What does it mean for Code Actions to work, but those commands to not work? What is an example of a Code Action that does work?

Sorry, I think the issue was that I've been using the term 'code actions' inter-changeably with 'quick fixes'. Quick Fixes are an example of a working Code Action - the only Code Actions that don't work yet should be Organize Imports and Fix All. I had a typo in the original limitations write-up that probably caused this confusion.

MichaReiser commented 3 months ago

Before I take a second look at the code changes: Would you mind updating the PR summary with a test plan?

snowsignal commented 3 months ago

To confirm, this will be merged into a separate branch to enable us to keep releasing from main?

Right. These changes will reside solely in the pre-release branch for now.

snowsignal commented 3 months ago

@MichaReiser I've added a test plan as requested.

snowsignal commented 3 months ago

@dhruvmanila

I don't think we should provide an error stating when the server isn't able to find the configuration file. This probably needs to be handled inside ruff.

Would it be okay to at least have a warning logged about this? I feel like this could be useful for catching issues with our configuration finder (instead of falling back silently)

We might want to clarify that we don't support source level code actions yet which means the editor.codeActionsOnSave config wouldn't work.

This is somewhat covered in the "we don't support extension configuration yet" bullet point, but I can revise it to point this out specifically.

MichaReiser commented 3 months ago

Wow, that's a very detailed test plan. Thank you! What I like for test plans that involve visual elements is to record a short video that shows how I perform the individual steps. I find it more convenient because I don't enjoy writing a lot and it allows reviews (or people who stumble across this PR in the future) to easily see how the VS code extension looked as of your PR.

snowsignal commented 3 months ago

@MichaReiser Got it! I'll use videos for future PRs, and I'll try to add a video to this test plan when I get the chance.

charliermarsh commented 3 months ago

@snowsignal - Def no need to add anything here, it's a great test plan! I think that's just advice for the future to save you time.