refactor CLI (2/3): Group options in `--help` #2155

not-my-profile · 2023-01-25T14:13:35Z

This is the followup PR for #2145.

New ruff --help output:

Ruff: An extremely fast Python linter.

Usage: ruff [OPTIONS] [FILES]...

Arguments:
  [FILES]...  

Options:
      --fix              Attempt to automatically fix lint violations
      --show-source      Show violations with source code
      --diff             Avoid writing any fixed files back; instead, output a diff for each changed file to stdout
  -w, --watch            Run in watch mode by re-running whenever files change
      --fix-only         Fix any fixable lint violations, but don't report on leftover violations. Implies `--fix`
      --format <FORMAT>  Output serialization format for violations [env: RUFF_FORMAT=] [possible values: text, json, junit, grouped, github, gitlab, pylint]
      --config <CONFIG>  Path to the `pyproject.toml` or `ruff.toml` file to use for configuration
      --add-noqa         Enable automatic additions of `noqa` directives to failing lines
      --show-files       See the files Ruff will be run against with the current settings
      --show-settings    See the settings Ruff will use to lint a given Python file
  -h, --help             Print help
  -V, --version          Print version

Rule selection:
      --select <RULE_CODE>
          Comma-separated list of rule codes to enable (or ALL, to enable all rules)
      --ignore <RULE_CODE>
          Comma-separated list of rule codes to disable
      --extend-select <RULE_CODE>
          Like --select, but adds additional rule codes on top of the selected ones
      --extend-ignore <RULE_CODE>
          Like --ignore, but adds additional rule codes on top of the ignored ones
      --per-file-ignores <PER_FILE_IGNORES>
          List of mappings from file pattern to code to exclude
      --fixable <RULE_CODE>
          List of rule codes to treat as eligible for autofix. Only applicable when autofix itself is enabled (e.g., via `--fix`)
      --unfixable <RULE_CODE>
          List of rule codes to treat as ineligible for autofix. Only applicable when autofix itself is enabled (e.g., via `--fix`)

File selection:
      --exclude <FILE_PATTERN>         List of paths, used to omit files and/or directories from analysis
      --extend-exclude <FILE_PATTERN>  Like --exclude, but adds additional files and directories on top of those already excluded
      --respect-gitignore              Respect file exclusions via `.gitignore` and other standard ignore files
      --force-exclude                  Enforce exclusions, even for paths passed to Ruff directly on the command-line

Rule configuration:
      --target-version <TARGET_VERSION>
          The minimum Python version that should be supported
      --line-length <LINE_LENGTH>
          Set the line-length for length-associated rules and automatic formatting
      --dummy-variable-rgx <DUMMY_VARIABLE_RGX>
          Regular expression matching the name of dummy variables

Miscellaneous:
  -n, --no-cache
          Disable cache reads
      --isolated
          Ignore all configuration files
      --cache-dir <CACHE_DIR>
          Path to the cache directory [env: RUFF_CACHE_DIR=]
      --stdin-filename <STDIN_FILENAME>
          The name of the file when passing it through stdin
  -e, --exit-zero
          Exit with status code "0", even upon detecting lint violations
      --update-check
          Enable or disable automatic update checks

Subcommands:
      --explain <EXPLAIN>  Explain a rule
      --clean              Clear any caches in the current directory or any subdirectories

Log levels:
  -v, --verbose  Enable verbose logging
  -q, --quiet    Print lint violations, but nothing else
  -s, --silent   Disable all logging (but still exit with status code "1" upon detecting lint violations)

charliermarsh · 2023-01-26T03:13:59Z

This is a significant improvement. And migrating to subcommands opens up a lot of opportunities

I've been hesitant to do this out of fear of breaking compatibility. But the setup you've achieved here is pretty interesting. I need some time to digest the impact and implications.

I do think we should have clear answers to a few questions that've come up before:

Do we expect ruff . to always work? That is, should check always be the "default" subcommand? Or is this just a temporarily measure, for compatibility?
Do we want to move ruff . --fix to ruff fix? Is there any value, at all, in that? deno has deno check (to type-check) and deno fmt to format, but no clear analog here. (I think the answer is "no", but now is the time to ask it.)

The idiomatic way in Rust is to make invalid types unrepresentable instead of paranoidly calling a validate method everywhere.

not-my-profile · 2023-01-26T05:49:32Z

I have updated this PR to only group the options in --help for now so that it can be merged and the subcommands can be implemented in a followup PR :) Please see the latest commit message for the reasoning.

To be clear this PR is now purely a refactor (aside from the changed --help output).

Do we expect ruff . to always work?

Yes I'd expect it to always work. I think it makes sense that a tool defaults to its main functionality. ruff is a linter so it makes sense that invoking it with some files/directories just "does the thing it's supposed to do" (linting).

Do we want to move ruff . --fix to ruff fix? Is there any value, at all, in that?

I think subcommands primarily make sense when they have a different synopsis, e.g:

--explain and --clean don't support any of the other options (=> they should be subcommands)
--fix and --add-noqa act like boolean flags (that ideally should be combinable, => option flags are fine)

Thanks for asking this question ... it made me realize that --add-noqa really shouldn't be a subcommand, since it would make sense to be able to combine it with --fix (even though that currently doesn't work at all, see #2187).
In the same vein I think --show-settings and --show-files shouldn't be subcommands since combining them with the other options would be useful, e.g. you should be able to see how --select or --no-respect-gitignore affects the selected rules / files.

charliermarsh · 2023-01-26T17:32:16Z

ruff_cli/src/args.rs

-    #[arg(long, conflicts_with = "config")]
-    pub isolated: bool,
+    #[clap(long, overrides_with("show_source"), hide = true)]
+    no_show_source: bool,


Should this not be grouped with show_source? Same for no_fix and fix.

Ah yes, good catch ... fixed :)

`ruff --help` previously listed 37 options in no particular order (with niche options like --isolated being listed before before essential options such as --select). This commit remedies that and additionally groups the options by making use of the Clap help_heading feature. Note that while the source code has previously also referred to --add-noqa, --show-settings, and --show-files as "subcommands" this commit intentionally does not list them under the new Subcommands section since contrary to --explain and --clean combining them with most of the other options makes sense.

charliermarsh · 2023-01-26T17:47:23Z

src/settings/mod.rs

+                    CARGO_PKG_VERSION
+                ));
+            }
+        }


Is the idea here that we now validate when reading the config, rather than when transforming it into Settings? I wonder why I didn't do it this way initially. I'm getting Chesterton's fence vibes.

I am sorry I cannot tell you why you wrote it this way ... looking at it just doesn't make sense to me ... as I explained in the commit message in Rust you ideally manage to make illegal states unrepresentable ... since Settings is used all over the codebase it only makes sense that Settings denotes valid settings as opposed to settings that may be invalid. So yes I think the Settings constructor is the right place for any settings validation.

Are you saying you don't like my code...? (Joking)

Haha ... no. I think it's very impressive that you managed to get ruff so far and keep grinding on the issues that matter. I tend to get sidetracked too much while pursuing my own projects ...

Thank you :) I was only kidding but maybe I'll just say a few quick things:

I try to stay unattached to my code so, of course, always feel free to suggest improvements.

To me much of building software is tradeoffs and prioritization... Ruff is a big project and so required making a lot of tradeoffs!

As a result, there's certainly "bad" code in various places, things that are done poorly that I know I'd like to be done differently, things that are done poorly that I've totally forgotten about, etc.

...and as such, I'm really appreciative of all the larger refactors you've taken on! They've made the project much better. Some of them aligned with things I wanted to change, some of them hadn't even occurred to me.

I know you were kidding^^

Right ... I just often don't like making tradeoffs and try to avoid them and end up going down some rabbit hole.

not-my-profile force-pushed the refactor-cli branch 2 times, most recently from 9716ad2 to 2953d29 Compare January 25, 2023 14:32

not-my-profile changed the title ~~refactor CLI (2/2)~~ refactor CLI (2/2): Use subcommands Jan 25, 2023

not-my-profile changed the title ~~refactor CLI (2/2): Use subcommands~~ refactor CLI (2/2): Use clap subcommands Jan 25, 2023

not-my-profile force-pushed the refactor-cli branch from 2953d29 to 961919e Compare January 25, 2023 14:45

not-my-profile added 5 commits January 26, 2023 04:21

refactor: Move Args::partition call after panic::set_hook

b235097

refactor: Introduce LogLevelArgs

8a496cb

refactor: Check required_version in Settings::from_configuration

0c6d53a

The idiomatic way in Rust is to make invalid types unrepresentable instead of paranoidly calling a validate method everywhere.

refactor: Get rid of ruff::resolver::FileDiscovery

5f9393e

refactor: Move add_noqa if branch up

9683ca2

not-my-profile force-pushed the refactor-cli branch 4 times, most recently from 5397553 to 468b31f Compare January 26, 2023 03:36

not-my-profile marked this pull request as draft January 26, 2023 04:29

not-my-profile force-pushed the refactor-cli branch from 468b31f to 414227a Compare January 26, 2023 05:38

not-my-profile changed the title ~~refactor CLI (2/2): Use clap subcommands~~ rework CLI (2/3): Group options in --help Jan 26, 2023

not-my-profile marked this pull request as ready for review January 26, 2023 05:40

not-my-profile changed the title ~~rework CLI (2/3): Group options in --help~~ refactor CLI (2/3): Group options in --help Jan 26, 2023

not-my-profile force-pushed the refactor-cli branch from 414227a to d238509 Compare January 26, 2023 05:54

This was referenced Jan 26, 2023

Add support for rule aliasing #2186

Open

rework CLI (3/3): Use clap subcommands #2190

Merged

charliermarsh reviewed Jan 26, 2023

View reviewed changes

not-my-profile force-pushed the refactor-cli branch from d238509 to d8ccfc1 Compare January 26, 2023 17:40

charliermarsh reviewed Jan 26, 2023

View reviewed changes

charliermarsh merged commit 23819ae into astral-sh:main Jan 26, 2023

not-my-profile mentioned this pull request Feb 1, 2023

Improve cargo insta help output mitsuhiko/insta#342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor CLI (2/3): Group options in `--help` #2155

refactor CLI (2/3): Group options in `--help` #2155

not-my-profile commented Jan 25, 2023 •

edited

Loading

charliermarsh commented Jan 26, 2023

not-my-profile commented Jan 26, 2023 •

edited

Loading

charliermarsh Jan 26, 2023

not-my-profile Jan 26, 2023

charliermarsh Jan 26, 2023

not-my-profile Jan 26, 2023 •

edited

Loading

charliermarsh Jan 26, 2023

not-my-profile Jan 26, 2023

charliermarsh Jan 26, 2023

not-my-profile Jan 26, 2023

refactor CLI (2/3): Group options in --help #2155

refactor CLI (2/3): Group options in --help #2155

Conversation

not-my-profile commented Jan 25, 2023 • edited Loading

charliermarsh commented Jan 26, 2023

not-my-profile commented Jan 26, 2023 • edited Loading

charliermarsh Jan 26, 2023

Choose a reason for hiding this comment

not-my-profile Jan 26, 2023

Choose a reason for hiding this comment

charliermarsh Jan 26, 2023

Choose a reason for hiding this comment

not-my-profile Jan 26, 2023 • edited Loading

Choose a reason for hiding this comment

charliermarsh Jan 26, 2023

Choose a reason for hiding this comment

not-my-profile Jan 26, 2023

Choose a reason for hiding this comment

charliermarsh Jan 26, 2023

Choose a reason for hiding this comment

not-my-profile Jan 26, 2023

Choose a reason for hiding this comment

refactor CLI (2/3): Group options in `--help` #2155

refactor CLI (2/3): Group options in `--help` #2155

not-my-profile commented Jan 25, 2023 •

edited

Loading

not-my-profile commented Jan 26, 2023 •

edited

Loading

not-my-profile Jan 26, 2023 •

edited

Loading