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

Allow arbitrary configuration options to be overridden via the CLI #9599

Merged
merged 61 commits into from
Feb 9, 2024

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jan 21, 2024

Fixes #8368
Fixes #9186

Summary

Arbitrary TOML strings can be provided via the command-line to override configuration options in pyproject.toml or ruff.toml. As an example: to run over typeshed and respect typeshed's pyproject.toml, but override a specific isort setting and enable an additional pep8-naming setting:

cargo run -- check ../typeshed --no-cache --config ../typeshed/pyproject.toml --config "lint.isort.combine-as-imports=false" --config "lint.extend-select=['N801']"

This is what the error message currently looks like if you provide an invalid --config option:

>cargo run -- check ../typeshed --no-cache --config foo.toml
    Finished dev [unoptimized + debuginfo] target(s) in 0.77s
     Running `target\debug\ruff.exe check ../typeshed --no-cache --config foo.toml`
error: invalid value 'foo.toml' for '--config <CONFIG_OPTION>'

  tip: The `--config` flag must either be a path to a `.toml` configuation file or a TOML string providing configuration overrides
  tip: The path `foo.toml` does not exist on your filesystem

For more information, try '--help'.
error: process didn't exit successfully: `target\debug\ruff.exe check ../typeshed --no-cache --config foo.toml` (exit code: 2)

Test Plan

So far, just manual testing. TODO: write proper tests and docs.

Several tests added to crates/ruff/tests/format.rs and crates/ruff/tests/lint.rs. Also, extensive manual testing.

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! I haven't done a full review and only skimmed through the code.

Some CLI tests showing how overrides work for check and format, including some advanced use cases like specifying values for a table (e.g is there a way to "append" to --per-file-ignores without having to use a JSON value?) Can I do --config "per-file-ignores.__init__.py" = ["RUF100"]? See ruff/tests/format or /ruff/tests/lint etc for examples.

crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff_workspace/src/configuration.rs Outdated Show resolved Hide resolved
crates/ruff_workspace/src/options.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.

Nice work!

crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
crates/ruff_workspace/src/options.rs Show resolved Hide resolved
crates/ruff_workspace/src/options.rs Show resolved Hide resolved
crates/ruff/src/args.rs Outdated Show resolved Hide resolved
BurntSushi added a commit that referenced this pull request Jan 24, 2024
Previously, without the 'wrap_help' feature enabled, Clap would not do
any auto-wrapping of help text. For help text with long lines, this
tends to lead to non-ideal formatting. It can be especially difficult to
read when the width of the terminal is smaller.

This commit enables 'wrap_help', which will automatically cause Clap to
query the terminal size and wrap according to that. Or, if the terminal
size cannot be determined, it will default to a maximum line width of
100.

Ref #9599 (comment)
BurntSushi added a commit that referenced this pull request Jan 24, 2024
Previously, without the 'wrap_help' feature enabled, Clap would not do
any auto-wrapping of help text. For help text with long lines, this
tends to lead to non-ideal formatting. It can be especially difficult to
read when the width of the terminal is smaller.

This commit enables 'wrap_help', which will automatically cause Clap to
query the terminal size and wrap according to that. Or, if the terminal
size cannot be determined, it will default to a maximum line width of
100.

Ref #9599 (comment)
BurntSushi added a commit that referenced this pull request Jan 24, 2024
Previously, without the 'wrap_help' feature enabled, Clap would not do
any auto-wrapping of help text. For help text with long lines, this
tends to lead to non-ideal formatting. It can be especially difficult to
read when the width of the terminal is smaller.

This commit enables 'wrap_help', which will automatically cause Clap to
query the terminal size and wrap according to that. Or, if the terminal
size cannot be determined, it will default to a maximum line width of
100.

Ref #9599 (comment)
BurntSushi added a commit that referenced this pull request Jan 24, 2024
Previously, without the 'wrap_help' feature enabled, Clap would not do
any auto-wrapping of help text. For help text with long lines, this
tends to lead to non-ideal formatting. It can be especially difficult to
read when the width of the terminal is smaller.

This commit enables 'wrap_help', which will automatically cause Clap to
query the terminal size and wrap according to that. Or, if the terminal
size cannot be determined, it will default to a maximum line width of
100.

Ref #9599 (comment)
Copy link
Contributor

github-actions bot commented Jan 24, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Formatter (stable)

✅ ecosystem check detected no format changes.

Formatter (preview)

✅ ecosystem check detected no format changes.

Copy link

codspeed-hq bot commented Jan 25, 2024

CodSpeed Performance Report

Merging #9599 will not alter performance

Comparing AlexWaygood:arbitrary-cli-overrides (0f8a583) with main (d387d0b)

Summary

✅ 30 untouched benchmarks

@AlexWaygood
Copy link
Member Author

Thanks so much @zanieb! I think I tackled nearly all of your points, except for #9599 (comment) where we're waiting for Charlie to chime in.

Some of the error messages might still be a bit verbose for your liking, but I'm not sure how to trim them down... suggestions welcome :)

@charliermarsh
Copy link
Member

Sorry, didn't realize this was waiting on me! I'll take a look today, then we should merge :)

docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated Show resolved Hide resolved
docs/configuration.md Outdated 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.

Looks great overall.


// small hack so that multiline tips
// have the same indent on the left-hand side:
let tip_indent = " ".repeat(" tip: ".len());
Copy link
Member

Choose a reason for hiding this comment

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

A little concerning how much implicit coupling there is to Clap, but I don't have better suggestions (and at least there's test coverage, I think, such that if we upgrade Clap and the output format changes, we will know that there's a disconnect here).

Copy link
Member Author

@AlexWaygood AlexWaygood Feb 9, 2024

Choose a reason for hiding this comment

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

Yeah. Ideally I feel like clap would have a feature that allows long error messages to be wrapped to the terminal width, much as it does with the output of --help. But it doesn't seem to have that feature right now :/

Initially I had each sentence as a separate tip, which obviated the need for this kind of hack, but Zanie (correctly!) pointed out that that looked pretty weird

@AlexWaygood
Copy link
Member Author

Thanks all for the reviews! I'll land this now

@AlexWaygood AlexWaygood enabled auto-merge (squash) February 9, 2024 21:18
@AlexWaygood AlexWaygood merged commit 8ec5627 into astral-sh:main Feb 9, 2024
17 checks passed
@AlexWaygood AlexWaygood deleted the arbitrary-cli-overrides branch February 9, 2024 21:56
@ofek
Copy link
Contributor

ofek commented Feb 19, 2024

For posterity: #10035

charliermarsh added a commit that referenced this pull request Mar 4, 2024
## Summary

When users provide configurations via `--config`, we use `shellexpand`
to ensure that we expand signifiers like `~` and environment variables.

In #9599, we modified `--config`
to accept either a path or an arbitrary setting. However, the detection
(to determine whether the value is a path or a setting) was lacking the
`shellexpand` behavior -- it was downstream. So we were always treating
paths like `~/ruff.toml` as values, not paths.

Closes astral-sh/ruff-vscode#413.
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
…stral-sh#9599)

Fixes astral-sh#8368
Fixes astral-sh#9186

## Summary

Arbitrary TOML strings can be provided via the command-line to override
configuration options in `pyproject.toml` or `ruff.toml`. As an example:
to run over typeshed and respect typeshed's `pyproject.toml`, but
override a specific isort setting and enable an additional pep8-naming
setting:

```
cargo run -- check ../typeshed --no-cache --config ../typeshed/pyproject.toml --config "lint.isort.combine-as-imports=false" --config "lint.extend-select=['N801']"
```

---------

Co-authored-by: Micha Reiser <micha@reiser.io>
Co-authored-by: Zanie Blue <contact@zanie.dev>
nkxxll pushed a commit to nkxxll/ruff that referenced this pull request Mar 10, 2024
## Summary

When users provide configurations via `--config`, we use `shellexpand`
to ensure that we expand signifiers like `~` and environment variables.

In astral-sh#9599, we modified `--config`
to accept either a path or an arbitrary setting. However, the detection
(to determine whether the value is a path or a setting) was lacking the
`shellexpand` behavior -- it was downstream. So we were always treating
paths like `~/ruff.toml` as values, not paths.

Closes astral-sh/ruff-vscode#413.
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
6 participants