`--show-settings` displays active settings in a far more readable format #9464

snowsignal · 2024-01-11T09:20:59Z

Summary

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

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.
Several complex types were also stubbed out and need proper Display implementations rather than falling back on Debug.
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.
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!.

github-actions · 2024-01-11T09:39:08Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+0 -1 violations, +0 -0 fixes in 1 projects; 42 projects unchanged)

bokeh/bokeh (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --no-preview --select ALL

- src/bokeh/core/serialization.py:666:34: PD002 `inplace=True` should be avoided; it has inconsistent behavior

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PD002	1	0	1	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+0 -1 violations, +0 -0 fixes in 1 projects; 42 projects unchanged)

bokeh/bokeh (+0 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --preview --select ALL

- src/bokeh/core/serialization.py:666:34: PD002 `inplace=True` should be avoided; it has inconsistent behavior

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
PD002	1	0	1	0	0

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

- A new utility macro, `display_settings`, has been created to reduce boilerplate for displaying settings fields - `Display` has been implemented on many custom types used in `ruff_workspace::Settings` - `--show-settings` now uses `Display` instead of `Debug` on `ruff_workspace::Settings` Work left to be done: - A lot of formatting for Vec<_> and HashSet<_> types has 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`. - Several complex types were also stubbed out and need proper `Display` implementations rather than falling back on `Debug` - An open question needs to be answered: should this output be valid TOML? How important is that?

MichaReiser

Nice work!

I've left a few nit comments that are up to you if you want to address them.

My main question is what convention we want to use for the field names.

The option names in the YAML configuration and the CLI use kebab case: extend-ignore.

I see reasons for using the same as well as for using different names than what we use in the configuration:

Same names: More familiar to users
Different names: Makes it clear that these are the resolved settings which doesn't map 1:1 to the YAML or CLI configuration. For example, the resolved settings can have fewer or more fields than the options, the values are merged from different settings (or sections) etc.

Before you publish your PR. Could you expand your test plan with an example output?

crates/ruff_linter/src/registry/rule_set.rs

crates/ruff_linter/src/rules/isort/categorize.rs

crates/ruff_linter/src/settings/mod.rs

BurntSushi

Wow, that's a lot of Display impls! I agree that your display_settings! macro here is pretty effective.

Very nice work. :)

crates/ruff_formatter/src/lib.rs

crates/ruff_linter/src/settings/mod.rs

crates/ruff_linter/src/rules/flake8_import_conventions/settings.rs

…ove the `.` at the end of each

… `FilePatternSet` and `RuleSet`

charliermarsh

This is great work! Big improvement.

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.

In my opinion, this is a nice-to-have but not critical, since the representation isn't going to match the representation that users have in their own pyproject.toml or ruff.toml file (since Settings is an internal representation, rather than the user-facing representation).

crates/ruff_linter/src/registry/rule_set.rs

crates/ruff_linter/src/rules/flake8_copyright/settings.rs

crates/ruff_linter/src/settings/types.rs

…d remove debug formatter for `KnownModules`

…ings`

…y_settings` macro

codspeed-hq · 2024-01-11T19:52:06Z

CodSpeed Performance Report

Merging #9464 will degrade performances by 4.35%

_{Comparing InquisitivePenguin:jane/settings/display-improvements (a24a50a) with main (fee64b5)}

Summary

❌ 1 regressions
✅ 29 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

	Benchmark	`main`	`InquisitivePenguin:jane/settings/display-improvements`	Change
❌	`parser[numpy/ctypeslib.py]`	12.2 ms	12.8 ms	-4.35%

charliermarsh · 2024-01-11T20:00:03Z

(Feel free to ignore the perf regression - you may need to merge / rebase on main, or otherwise we may be seeing some flakiness (in other PRs too).)

…isplay-improvements

snowsignal · 2024-01-11T20:10:05Z

I'll merge from main just in case 🙂

charliermarsh

This is great work!

crates/ruff_cli/tests/snapshots/show_settings__display_default_settings.snap

crates/ruff_linter/src/settings/mod.rs

MichaReiser

So many display implementations 😆

This is excellent. I prefer to have some empty lines between the sections, but I'll leave the decision up to you. Let me (or anyone else around at the time) know when it is ready to merge.

crates/ruff_cli/tests/snapshots/show_settings__display_default_settings.snap

crates/ruff_linter/src/settings/types.rs

konstin

Great work!

crates/ruff_linter/src/settings/mod.rs

crates/ruff_linter/src/settings/types.rs

…isplay-improvements

charliermarsh · 2024-01-12T19:30:24Z

Rock on

MichaReiser added the cli Related to the command-line interface label Jan 11, 2024

snowsignal force-pushed the jane/settings/display-improvements branch from 69b887e to 651c1aa Compare January 11, 2024 09:59

MichaReiser reviewed Jan 11, 2024

View reviewed changes

crates/ruff_linter/src/registry/rule_set.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/isort/categorize.rs Show resolved Hide resolved

crates/ruff_linter/src/settings/mod.rs Show resolved Hide resolved

BurntSushi reviewed Jan 11, 2024

View reviewed changes

snowsignal added 3 commits January 11, 2024 08:02

Use Display::fmt directly instead of write!(f, {}, ...)

1341d50

Make namespace an optional parameter for display_settings and rem…

415104e

…ove the `.` at the end of each

Eliminate unnessecary allocations in the Display implementations of…

afe986c

… `FilePatternSet` and `RuleSet`

charliermarsh reviewed Jan 11, 2024

View reviewed changes

crates/ruff_linter/src/registry/rule_set.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/flake8_copyright/settings.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/settings/types.rs Show resolved Hide resolved

snowsignal added 3 commits January 11, 2024 08:17

Remove unnessecary allocation from impl Display for KnownModules an…

16dbd8f

…d remove debug formatter for `KnownModules`

Implement array and optional formatting options for `display_sett…

e7a6aab

…ings`

Add quoted formatter to display_settings and document the `displa…

eb9c1fb

…y_settings` macro

Properly implement Display for PerFileIgnores

0ce38bc

Merge branch 'main' of github.com:astral-sh/ruff into jane/settings/d…

2856361

…isplay-improvements

snowsignal marked this pull request as ready for review January 11, 2024 22:09

Set up a snapshot test for the default output of --show-settings

14221b6

snowsignal force-pushed the jane/settings/display-improvements branch from 685ea76 to 14221b6 Compare January 11, 2024 22:15

Remove extraneous newline from stdout for --show-settings

84a1a2c

snowsignal force-pushed the jane/settings/display-improvements branch 2 times, most recently from 644aa55 to a8fd56c Compare January 11, 2024 23:58

Cleanup snapshot tests

14a8a6e

snowsignal force-pushed the jane/settings/display-improvements branch from a8fd56c to 14a8a6e Compare January 12, 2024 00:09

charliermarsh approved these changes Jan 12, 2024

View reviewed changes

crates/ruff_cli/tests/snapshots/show_settings__display_default_settings.snap Show resolved Hide resolved

crates/ruff_linter/src/settings/mod.rs Show resolved Hide resolved

crates/ruff_linter/src/settings/mod.rs Outdated Show resolved Hide resolved

snowsignal added 2 commits January 11, 2024 16:31

Add top-level comment to Settings display

8c82e87

Make resolve_per_file_ignores into an associated function

5793b60

MichaReiser approved these changes Jan 12, 2024

View reviewed changes

konstin approved these changes Jan 12, 2024

View reviewed changes

crates/ruff_linter/src/settings/mod.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/settings/types.rs Show resolved Hide resolved

snowsignal added 5 commits January 12, 2024 11:02

Space out comment headers for settings display

142883c

Add Linter Plugins heading to settings display

b3e3c39

Use 'none' instead of 'nil' when displaying None values in settings

27007a1

Explain the order of matchers in PerFileIgnores

ba3e00b

Merge branch 'main' of github.com:astral-sh/ruff into jane/settings/d…

a24a50a

…isplay-improvements

charliermarsh merged commit 7504bf3 into astral-sh:main Jan 12, 2024
16 of 17 checks passed

snowsignal deleted the jane/settings/display-improvements branch January 12, 2024 19:31

branchvincent mentioned this pull request Jan 19, 2024

ruff 0.1.14 Homebrew/homebrew-core#160455

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`--show-settings` displays active settings in a far more readable format #9464

`--show-settings` displays active settings in a far more readable format #9464

snowsignal commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

MichaReiser left a comment •

edited

Loading

BurntSushi left a comment

charliermarsh left a comment

codspeed-hq bot commented Jan 11, 2024 •

edited

Loading

charliermarsh commented Jan 11, 2024

snowsignal commented Jan 11, 2024

charliermarsh left a comment

MichaReiser left a comment

konstin left a comment

charliermarsh commented Jan 12, 2024

--show-settings displays active settings in a far more readable format #9464

--show-settings displays active settings in a far more readable format #9464

Conversation

snowsignal commented Jan 11, 2024 • edited Loading

Summary

Work to be done

Test Plan

github-actions bot commented Jan 11, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

Formatter (stable)

Formatter (preview)

MichaReiser left a comment • edited Loading

Choose a reason for hiding this comment

BurntSushi left a comment

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

codspeed-hq bot commented Jan 11, 2024 • edited Loading

CodSpeed Performance Report

Merging #9464 will degrade performances by 4.35%

Summary

Benchmarks breakdown

charliermarsh commented Jan 11, 2024

snowsignal commented Jan 11, 2024

charliermarsh left a comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

konstin left a comment

Choose a reason for hiding this comment

charliermarsh commented Jan 12, 2024

`--show-settings` displays active settings in a far more readable format #9464

`--show-settings` displays active settings in a far more readable format #9464

snowsignal commented Jan 11, 2024 •

edited

Loading

github-actions bot commented Jan 11, 2024 •

edited

Loading

`ruff-ecosystem` results

MichaReiser left a comment •

edited

Loading

codspeed-hq bot commented Jan 11, 2024 •

edited

Loading