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

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

Merged

Conversation

snowsignal
Copy link
Contributor

@snowsignal snowsignal commented Jan 11, 2024

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

  • 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!.

@MichaReiser MichaReiser added the cli Related to the command-line interface label Jan 11, 2024
Copy link
Contributor

github-actions bot commented Jan 11, 2024

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?
@snowsignal snowsignal force-pushed the jane/settings/display-improvements branch from 69b887e to 651c1aa Compare January 11, 2024 09:59
Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
crates/ruff_linter/src/settings/mod.rs Show resolved Hide resolved
Copy link
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
crates/ruff_linter/src/settings/mod.rs Show resolved Hide resolved
crates/ruff_linter/src/settings/mod.rs Show resolved Hide resolved
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.

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

codspeed-hq bot commented Jan 11, 2024

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

(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).)

@snowsignal
Copy link
Contributor Author

I'll merge from main just in case 🙂

@snowsignal snowsignal marked this pull request as ready for review January 11, 2024 22:09
@snowsignal snowsignal force-pushed the jane/settings/display-improvements branch from 685ea76 to 14221b6 Compare January 11, 2024 22:15
@snowsignal snowsignal force-pushed the jane/settings/display-improvements branch 2 times, most recently from 644aa55 to a8fd56c Compare January 11, 2024 23:58
@snowsignal snowsignal force-pushed the jane/settings/display-improvements branch from a8fd56c to 14a8a6e Compare January 12, 2024 00:09
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.

This is great work!

Copy link
Member

@MichaReiser MichaReiser left a comment

Choose a reason for hiding this comment

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

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.

Copy link
Member

@konstin konstin left a comment

Choose a reason for hiding this comment

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

Great work!

crates/ruff_linter/src/settings/mod.rs Outdated Show resolved Hide resolved
crates/ruff_linter/src/settings/types.rs Show resolved Hide resolved
@charliermarsh
Copy link
Member

Rock on

@charliermarsh charliermarsh merged commit 7504bf3 into astral-sh:main Jan 12, 2024
16 of 17 checks passed
@snowsignal snowsignal deleted the jane/settings/display-improvements branch January 12, 2024 19:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cli Related to the command-line interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create Display implementation for Settings
5 participants