-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Create Display
implementation for Settings
#8334
Comments
I agree. Do you think we should display it in a similar format |
Not sure. TOML format is probably more work and might be misleading because users could assume it is the loaded configuration as stored on disk whereas it is the resolved/merged configuration. We would also need to retain whether the user used That's why I would prefer a format that's different enough for users to understand it is not their |
Related: #8396 |
There's also some discussion on this interface in my internal CLI document |
I see you've mentioned human readable format but do we also want it to be machine readable? Maybe it could be useful in some integrations. Personally, I find the YAML format easier to read in such scenarios which showcases the hierarchy and the with:
repository: astral-sh/ruff
token: ***
ssh-strict: true
# ...
env:
CARGO_INCREMENTAL: 0
CARGO_NET_RETRY: 10
# ... |
I think for this use-case a flat display would be best e.g.
This would be valid toml but different from the format generally used by users. |
Maybe it's just me but I find that there are too many duplications in options at the same hierarchy 😅 fix: true
format:
preview: false
line-width: 88
... Although this does mean that for options common to multiple sections, the section name is above a few lines where the flat display would be much better. At the end, I'm fine with anything as long as it's generally readable :) |
Yeah I worry the section name is lost in that format and the most important use-case for me is actually to be able to grep the result for the setting I care about which does not work with nesting. |
I'm looking into this - my plan is to use @zanieb's formatting suggestion. Having a nested display would look nice, but I think |
…mat (#9464) <!-- Thank you for contributing to Ruff! To help us out with reviewing, please consider the following: - Does this pull request include a summary of the change? (See below.) - Does this pull request include a descriptive title? - Does this pull request include references to any relevant issues? --> ## Summary Fixes #8334. `Display` has been implemented for `ruff_workspace::Settings`, which gives a much nicer and more readable output to `--show-settings`. Internally, a `display_settings` utility macro has been implemented to reduce the boilerplate of the display code. ### Work to be done - [x] A lot of formatting for `Vec<_>` and `HashSet<_>` types have been stubbed out, using `Debug` as a fallback. There should be a way to add generic formatting support for these types as a modifier in `display_settings`. - [x] Several complex types were also stubbed out and need proper `Display` implementations rather than falling back on `Debug`. - [x] An open question needs to be answered: how important is it that the output be valid TOML? Some types in settings, such as a hash-map from a glob pattern to a multi-variant enum, will be hard to rework into valid _and_ readable TOML. - [x] Tests need to be implemented. ## Test Plan Tests consist of a snapshot test for the default `--show-settings` output and a doctest for `display_settings!`.
ruff check . --show-settings
prints the settings using theDebug
implementation. The result is that it prints our internal representation, which is hard to read (even impossible)ruff/crates/ruff_cli/src/commands/show_settings.rs
Line 38 in fe485d7
Current output
We should instead use the Setting's (not yet existing)
Display
implementation and ensure we print human readable values for each setting.The text was updated successfully, but these errors were encountered: