-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Expand ruff.configuration
to allow inline config
#16296
Conversation
|
ruff.configuration
to allow all configruff.configuration
to allow inline config
e8f2911
to
df9fbdc
Compare
Does this mean the following existing settings will be deprecated? |
df9fbdc
to
4b26a73
Compare
No, I don't think so we'll deprecate them. It's similar to how there's |
settings.configuration.as_ref().and_then(|configuration| { | ||
match ResolvedConfiguration::try_from(configuration) { | ||
Ok(configuration) => Some(configuration), | ||
Err(err) => { | ||
tracing::error!("Failed to resolve configuration: {err}"); | ||
None | ||
} | ||
} | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm planning to open a follow-up PR to notify the user that resolving the client settings failed. We currently don't log or notify the user if it fails. For example, lint.select = ["RUF07", "I001"]
will see that RUF07
fails and thus ignores this config value which means that I001
is not selected either.
I need to update the documentation but I'll wait to first make sure that this is the approach that we want to take. I'd appreciate getting an initial feedback for the approach, I've documented the limitations along with potential solutions that could be implemented to resolve them. |
I'll reply to some of the design questions before taking a look at the code itself.
One consequence of this is that it won't be possible to specify both a configuration file and individual configuration options, which is different from the CLI. I mainly want to call this out but I think this is an okay limitation because we could always introduce a The casing difference is interesting. I don't think we should change the casing of configuration options because they're then different from what we list in the documentation. Again, I think this limitation is fine.
I'm not sure I fully understand this limitation. I believe all field and table identifiers have to be string. It's only the values that can be numbers, strings, or entire tables. Could we, therefore, deserialize the settings to |
Yes, I'm aware of this limitation and as you mention, it wouldn't be too much of an effort to add
👍
That won't work. So, currently this is what happens:
The JSON string in your example would be:
which gets deserialized into a JSON map:
Remember that in JSON all keys are strings. Then, we create a TOML table and deserialize it into toml::Table::try_from(map)?.try_into::<Options>()?; Here, the |
Yeah, I think we would have to do the same as on the CLI where we parse the setting name as well. Simply deserializing into |
I took a look at Ruff's CLI and it seems to directly deserialize into the Table. I still think that it should be possible, but it may require some post/pre processing on our side. I'd have look closer into how TOML deserializes tables (which are thin wrappers around |
There's a small difference between the CLI and what we have here. The command-line value if already a TOML string which means it can just use What you're describing is then what I've laid down with "Nesting > Possible solution (1)" section where we'll have to create this TOML string ourselves from the JSON value. Does that make sense? I know it's a bit confusing 😅 |
(Oops, I didn't see your comment)
Fair enough. I tried a few things but that didn't work, I can time box it and look into how the library deserializes tables. |
Okay. I played a bit with Do you want me to review the code as well or did you mainly want to get feedback on the config format? |
Thank you for spending time looking into it. I also think this could possibly be done in the future if we get user feedback on this specific format issue because it would still be backwards compatible in that both nested and flat keys would be supported but right now it's only nested keys.
I mainly wanted to get feedback on the format. I'm planning to work on the documentation part tomorrow morning. I think it be would best to defer the review for tomorrow to save review cycle. |
4b26a73
to
db5a5e4
Compare
db5a5e4
to
cda5a52
Compare
I hope you don't mind if I put this back into draft. It makes it easier for me to see when I'm supposed to review it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. This looks great. I've mainly some documentation and error handling suggestions.
pub(crate) enum ResolvedConfigurationError { | ||
#[error(transparent)] | ||
EnvVarLookupError(#[from] shellexpand::LookupError<std::env::VarError>), | ||
#[error("error serializing configuration to TOML: {0}")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's unclear to me where we serialize the value to TOML. I understand deserialization but I don't see where we perform any serialization.
I worry that this error message is also not very helpful for users because it's not clear what's going wrong. Can we use a more specific error message? Failed to load configuration from ruff.configurations
:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The serialization of map
to TOML happens here (try_from
):
let options = toml::Table::try_from(map)?.try_into::<Options>()?; |
I'll update the error message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to keep this message for now. The "Failed to load ..." message that you've suggested is already going to be appended before this. The reason I want to keep this is that for something like:
"ruff.configuration": {
"line-length": null
},
We'll get:
Failed to load settings from `configuration`: unsupported unit type
But, with the above message:
Failed to load settings from `configuration`: error serializing configuration to TOML: unsupported unit type
We'll atleast know that the issue is with TOML deserialization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Failed to load settings from
configuration
: error serializing configuration to TOML: unsupported unit type
I don't think I would understand this message or even know what I have to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think one solution would be to handle these null
cases because that's the only case where I'm able to get this serialization error. I checked the spec as well (https://toml.io/en/v1.0.0#keys) but can't get the InvalidToml
error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could skip the intermediate JSON deserialization but that would yield to even worse experience:
error: Failed to deserialize initialization options: data did not match any variant of untagged enum InitializationOptions. Falling back to default client settings...
One noteworthy drawback of having a single option for |
Yeah, that's a good point. Maybe it might be worth adding the I was also wondering if something like |
I'm fine deferring it for now. I just thought it worth calling out. I expect that many users will want to use the inline config anyway |
## Summary Closes: #6 This PR adds support for astral-sh/ruff#16296 for the VS Code extension by allowing any arbitrary object in `ruff.configuration` Additionally, it will provide a warning message to the user if they're using inline configuration but the Ruff version does not support it. It has one disadvantage that the user will see two popups where the error one is coming from the server because it cannot deserialize the options. We could not show the warning popup if this is too much. I'm worried that it might go unnoticed because the "Show Logs" in the below popup will open the server logs while the message would be in the client logs. <img width="491" alt="Screenshot 2025-02-25 at 1 24 15 PM" src="https://github.com/user-attachments/assets/8bafbd69-f8fa-4604-8ab3-1f6efa745045" /> We'll still log it on the console (client log channel): ``` 2025-02-25 13:24:10.067 [warning] Inline configuration support was added in Ruff 0.9.8 (current version is 0.9.7). Please update your Ruff version to use this feature. ``` I haven't provided more details in the warning message based on the assumption that if the user is using inline configuration then they're aware of it but it's just that the Ruff version is too old. ## Test Plan Refer to the test plan in astral-sh/ruff#16296
* main: [red-knot] unify LoopState and saved_break_states (#16406) [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378) [FURB156] Do not consider docstring(s) (#16391) Use `is_none_or` in `stdlib-module-shadowing` (#16402) [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398) [red-knot] Fix file watching for new non-project files (#16395) document MSRV policy (#16384) [red-knot] fix non-callable reporting for unions (#16387) bump MSRV to 1.83 (#16294) Avoid unnecessary info at non-trace server log level (#16389) Expand `ruff.configuration` to allow inline config (#16296) Start detecting version-related syntax errors in the parser (#16090)
* dcreager/dont-have-a-cow: [red-knot] unify LoopState and saved_break_states (#16406) [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378) [FURB156] Do not consider docstring(s) (#16391) Use `is_none_or` in `stdlib-module-shadowing` (#16402) [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398) [red-knot] Fix file watching for new non-project files (#16395) document MSRV policy (#16384) [red-knot] fix non-callable reporting for unions (#16387) bump MSRV to 1.83 (#16294) Avoid unnecessary info at non-trace server log level (#16389) Expand `ruff.configuration` to allow inline config (#16296) Start detecting version-related syntax errors in the parser (#16090)
## Summary As mentioned in #16296 (comment) This PR updates the client settings resolver to notify the user if there are any errors in the config using a very basic approach. In addition, each error related to specific settings are logged. This isn't the best approach because it can log the same message multiple times when both workspace and global settings are provided and they both are the same. This is the case for a single workspace VS Code instance. I do have some ideas on how to improve this and will explore them during my free time (low priority): * Avoid resolving the global settings multiple times as they're static * Include the source of the setting (workspace or global?) * Maybe use a struct (`ResolvedClientSettings` + `Vec<ClientSettingsResolverError>`) instead to make unit testing easier ## Test Plan Using: ```jsonc { "ruff.logLevel": "debug", // Invalid settings "ruff.configuration": "$RANDOM", "ruff.lint.select": ["RUF000", "I001"], "ruff.lint.extendSelect": ["B001", "B002"], "ruff.lint.ignore": ["I999", "F401"] } ``` The error logs: ``` 2025-02-27 12:30:04.318736000 ERROR Failed to load settings from `configuration`: error looking key 'RANDOM' up: environment variable not found 2025-02-27 12:30:04.319196000 ERROR Failed to load settings from `configuration`: error looking key 'RANDOM' up: environment variable not found 2025-02-27 12:30:04.320549000 ERROR Unknown rule selectors found in `lint.select`: ["RUF000"] 2025-02-27 12:30:04.320669000 ERROR Unknown rule selectors found in `lint.extendSelect`: ["B001"] 2025-02-27 12:30:04.320764000 ERROR Unknown rule selectors found in `lint.ignore`: ["I999"] ``` Notification preview: <img width="470" alt="Screenshot 2025-02-27 at 12 29 06 PM" src="https://github.com/user-attachments/assets/61f41d5c-2558-46b3-a1ed-82114fd8ec22" />
* main: [red-knot] unify LoopState and saved_break_states (#16406) [`pylint`] Also reports `case np.nan`/`case math.nan` (`PLW0177`) (#16378) [FURB156] Do not consider docstring(s) (#16391) Use `is_none_or` in `stdlib-module-shadowing` (#16402) [red-knot] Upgrade salsa to include `AtomicPtr` perf improvement (#16398) [red-knot] Fix file watching for new non-project files (#16395) document MSRV policy (#16384) [red-knot] fix non-callable reporting for unions (#16387) bump MSRV to 1.83 (#16294) Avoid unnecessary info at non-trace server log level (#16389) Expand `ruff.configuration` to allow inline config (#16296) Start detecting version-related syntax errors in the parser (#16090)
Summary
Internal design document
This PR expands
ruff.configuration
to allow inline configuration directly in the editor. For example:This means that now
ruff.configuration
accepts either a path to configuration file or the raw config itself. It's mostly similar to--config
with one difference that's highlighted in the following section. So, it can be said that the format ofruff.configuration
when provided the config map is same as the one on the playground 1.Limitations
Casing (
kebab-case
v/s/camelCase
)The config keys needs to be in
kebab-case
instead ofcamelCase
which is being used for other settings in the editor.This could be a bit confusing. For example, the
line-length
option can be set directly via an editor setting or can be configured viaruff.configuration
:Possible solution
We could use feature flag with conditional compilation to indicate that when used in
ruff_server
, we need theOptions
fields to be renamed ascamelCase
while for other crates it needs to be renamed askebab-case
. But, this might not work very easily because it will require wrapping theOptions
struct and create two structs in which we'll have to add#[cfg_attr(...)]
because otherwiseserde
will complain:Nesting (flat v/s nested keys)
This is the major difference between
--config
flag on the command-line v/sruff.configuration
and it makes it such thatruff.configuration
has same value format as playground 1.The config keys needs to be split up into keys which can result in nested structure instead of flat structure:
So, the following won't work:
But, instead it would need to be split up like the following:
Possible solution (1)
The way we could solve this and make it same as
--config
would be to add a manual logic of converting the JSON map into an equivalent TOML string which would be then parsed intoOptions
.So, the following JSON map:
would need to be converted into the following TOML string:
by recursively convering
"key": value
intokey = value
which is to remove the quotes from key and replacing:
with=
.Possible solution (2)
Another would be to just accept
Map<String, String>
strictly and convert it intokey = value
and then parse it as a TOML string. This would also match--config
but quotes might become a nuisance because JSON only allows double quotes and so it'll require escaping any inner quotes or use single quotes.Test Plan
VS Code
Requires astral-sh/ruff-vscode#702
settings.json
:Following video showcases me doing the following:
TID
Ruff: Fix all auto-fixable problems
to testunfixable
Format: Document
to testline-length
andquote-style
Screen.Recording.2025-02-24.at.11.08.13.AM.mov
Neovim
init.lua
:Same steps as in the VS Code test:
Screen.Recording.2025-02-24.at.11.19.43.AM.mov
Documentation Preview
Screen.Recording.2025-02-26.at.10.15.55.AM.mov
Footnotes
This has one advantage that the value can be copy-pasted directly into the playground ↩ ↩2