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

ruff server: Support setting to prioritize project configuration over editor configuration #11086

Merged
merged 4 commits into from
Apr 26, 2024

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Apr 22, 2024

Summary

This is intended to address astral-sh/ruff-vscode#425, and is a follow-up to #11062.

A new client setting is now supported by the server, prioritizeFileConfiguration. This is a boolean setting (default: false) that, if set to true, will instruct the configuration resolver to prioritize file configuration (aka discovered TOML files) over configuration passed in by the editor.

A corresponding extension PR has been opened, which makes this setting available for VS Code: astral-sh/ruff-vscode#457.

Test Plan

To test this with VS Code, you'll need to check out the VS Code PR that adds this setting.

The test process is similar to #11062, but in scenarios where the editor configuration would take priority over file configuration, file configuration should take priority.

@snowsignal snowsignal added configuration Related to settings and configuration server Related to the LSP server labels Apr 22, 2024
@snowsignal snowsignal force-pushed the jane/server/settings/prioritize-workspace-settings branch from 9637979 to 86ff807 Compare April 22, 2024 03:13
@snowsignal snowsignal force-pushed the jane/server/settings/prioritize-workspace-settings branch from 86ff807 to 51b5c51 Compare April 22, 2024 03:24
@snowsignal snowsignal marked this pull request as ready for review April 22, 2024 03:24
Copy link
Contributor

github-actions bot commented Apr 22, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@snowsignal snowsignal force-pushed the jane/server/settings/resolution branch from 38b3b60 to afbfabe Compare April 23, 2024 18:06
Base automatically changed from jane/server/settings/resolution to main April 23, 2024 18:19
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be an enum, rather than a bool? That way, we can associate documentation with each variant too, which seems like it would be helpful.

@snowsignal
Copy link
Contributor Author

I wonder if this should be an enum, rather than a bool? That way, we can associate documentation with each variant too, which seems like it would be helpful.

@MichaReiser made a similar comment here. I think that's what I plan to go with :)

@charliermarsh
Copy link
Member

Oops, I swear I didn't see that!

@snowsignal
Copy link
Contributor Author

No worries 😄

@snowsignal snowsignal force-pushed the jane/server/settings/prioritize-workspace-settings branch from 51b5c51 to a71e3ed Compare April 25, 2024 00:31
@snowsignal snowsignal force-pushed the jane/server/settings/prioritize-workspace-settings branch from 2a2997f to f4ad8e8 Compare April 25, 2024 01:18
Copy link
Member

@charliermarsh charliermarsh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good, I'm just not sold on the names...

@charliermarsh
Copy link
Member

I think @MichaReiser will have good advice on the naming and documentation.

@MichaReiser
Copy link
Member

I think @MichaReiser will have good advice on the naming and documentation.

Lol no 😆 I commented in the other PR that I don't have good naming suggestions haha

crates/ruff_server/src/session/settings.rs Outdated Show resolved Hide resolved
crates/ruff_server/src/session/settings.rs Outdated Show resolved Hide resolved
…ce', rename variants, and introduce a third, editor-only variant.
@snowsignal snowsignal force-pushed the jane/server/settings/prioritize-workspace-settings branch from c8b6b56 to e58c8b6 Compare April 26, 2024 08:10
@snowsignal snowsignal enabled auto-merge (squash) April 26, 2024 08:12
@snowsignal snowsignal merged commit 16a1f3c into main Apr 26, 2024
18 checks passed
@snowsignal snowsignal deleted the jane/server/settings/prioritize-workspace-settings branch April 26, 2024 08:17
snowsignal added a commit to astral-sh/ruff-vscode 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 to astral-sh/ruff-vscode 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
configuration Related to settings and configuration server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants