search

astral-sh / ruff-vscode

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

Introduce common Ruff configuration options to extension settings #456

Closed snowsignal closed 2 months ago

snowsignal commented 2 months ago

Summary

This is a follow-up to https://github.com/astral-sh/ruff/pull/10984, which implemented support for common Ruff configuration options in client settings. This PR exposes these new settings in the extension configuration.

These settings are different from other extension settings, because they don't have default values. Instead, if the user does not set them, they will be sent as null, and the server will use project configuration instead.

At the moment, ruff server does not actually resolve these settings yet - settings resolution is introduced in https://github.com/astral-sh/ruff/pull/11062, which means that you'll need to work from that branch to test this PR (and vice-versa).

Test Plan

See the test plan in https://github.com/astral-sh/ruff/pull/11062.

MichaReiser commented 2 months ago

Could you please share a print screen how the settings are rendered in VS code?

What happens if you add settings to ruff.lint.select and then remove all of them again. Does it leave an empty list or does it reset the value to null?

snowsignal commented 2 months ago

@MichaReiser

Could you please share a print screen how the settings are rendered in VS code?

It's hard to get a picture of all the new settings, since they're split up across the configuration. Here's what a few of them look like (Lint > Run is not a new setting):

Screenshot 2024-04-22 at 9 09 19 PM

What happens if you add settings to ruff.lint.select and then remove all of them again. Does it leave an empty list or does it reset the value to null?

It leaves an empty list. VS Code indicates that the value is set by a blue highlight on it:

Screenshot 2024-04-22 at 9 11 10 PM

To make it null again, the user has to reset the setting from the dropdown menu. Which, to be fair, isn't particularly intuitive.

MichaReiser commented 2 months ago

Thanks for the screenshot. Can we influence the ordering of the settings? E.g., I'm surprised that lint.select, lint.extend-select, and lint.ignore aren't next to each other (when you probably want to configure them together).

To make it null again, the user has to reset the setting from the dropdown menu. Which, to be fair, isn't particularly intuitive.

Yeah, that's not ideal. Maybe the documentation should mention that they either need to reset the configuration or manually edit the JSON. However, I think that's fine for now. We can still change the field type to array | null in the future.

Having to click Add Item for each rule is a bit annoying but I guess users can navigate to the JSON editor themselves. So I think that UX is good.

snowsignal commented 2 months ago

Thanks for the screenshot. Can we influence the ordering of the settings? E.g., I'm surprised that lint.select, lint.extend-select, and lint.ignore aren't next to each other (when you probably want to configure them together).

I thought this wasn't possible but it looks like this was recently added as a feature. 😄 I'll add ordering in a follow-up.