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

Introduce common Ruff configuration options to extension settings #456

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

snowsignal
Copy link
Member

Summary

This is a follow-up to astral-sh/ruff#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 astral-sh/ruff#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 astral-sh/ruff#11062.

@MichaReiser
Copy link
Member

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?

src/common/settings.ts Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/common/settings.ts Outdated Show resolved Hide resolved
src/common/settings.ts Show resolved Hide resolved
…ntation links, scope new settings to 'resource', remove getLineLength functions
@snowsignal
Copy link
Member Author

@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
Copy link
Member

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 added a commit to astral-sh/ruff that referenced this pull request Apr 23, 2024
…ect configuration (#11062)

## Summary

This is a follow-up to #10984 that
implements configuration resolution for editor configuration. By 'editor
configuration', I'm referring to the client settings that correspond to
Ruff configuration/options, like `preview`, `select`, and so on. These
will be combined with 'project configuration' (configuration taken from
project files such as `pyproject.toml`) to generate the final linter and
formatter settings used by `RuffSettings`. Editor configuration takes
priority over project configuration.

In a follow-up pull request, I'll implement a new client setting that
allows project configuration to override editor configuration, as per
[this issue](astral-sh/ruff-vscode#425).

## Review guide

The first commit, e38966d, is just
doing re-arrangement so that we can pass the right things to
`RuffSettings::resolve`. The actual resolution logic is in the second
commit, 0eec9ee. It might help to look
at these comments individually since the diff is rather messy.

## Test Plan

For the settings to show up in VS Code, you'll need to checkout this
branch: astral-sh/ruff-vscode#456.

To test that the resolution for a specific setting works as expected,
run through the following scenarios, setting it in project and editor
configuration as needed:

| Set in project configuration? | Set in editor configuration? |
Expected Outcome |

|-------------------------------|--------------------------------------------------|------------------------------------------------------------------------------------------|
| No | No | The editor should behave as if the setting was set to its
default value. |
| Yes | No | The editor should behave as if the setting was set to the
value in project configuration. |
| No | Yes | The editor should behave as if the setting was set to the
value in editor configuration. |
| Yes | Yes (but distinctive from project configuration) | The editor
should behave as if the setting was set to the value in editor
configuration. |

An exception to this is `extendSelect`, which does not have an analog in
TOML configuration. Instead, you should verify that `extendSelect`
amends the `select` setting. If `select` is set in both editor and
project configuration, `extendSelect` will only append to the `select`
value in editor configuration, so make sure to un-set it there if you're
testing `extendSelect` with `select` in project configuration.
@snowsignal
Copy link
Member Author

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.

@snowsignal snowsignal merged commit e14932d into pre-release Apr 25, 2024
@snowsignal snowsignal deleted the jane/top-level-settings branch April 25, 2024 00:51
snowsignal added a commit that referenced this pull request May 13, 2024
## Summary

This is a follow-up to astral-sh/ruff#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
astral-sh/ruff#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 astral-sh/ruff#11062.
snowsignal added a commit that referenced this pull request May 17, 2024
)

## Summary

Fixes #425 (though only
in the pre-release version, since this is a feature for the new LSP
server)

This is a follow-up to
#456.

A new setting has been introduced, `Prioritize File Configuration`,
which tells the extension to prioritize settings from discovered TOML
files over extension settings. Extension settings will still be used
when a set field in the extension is not set in the file configuration.

## Test Plan

Refer to astral-sh/ruff#11086 for a test plan.
snowsignal added a commit that referenced this pull request May 17, 2024
)

## Summary

Fixes #425 (though only
in the pre-release version, since this is a feature for the new LSP
server)

This is a follow-up to
#456.

A new setting has been introduced, `Prioritize File Configuration`,
which tells the extension to prioritize settings from discovered TOML
files over extension settings. Extension settings will still be used
when a set field in the extension is not set in the file configuration.

## Test Plan

Refer to astral-sh/ruff#11086 for a test plan.
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.

None yet

2 participants