refactor CLI (2/3): Group options in --help#2155
refactor CLI (2/3): Group options in --help#2155charliermarsh merged 6 commits intoastral-sh:mainfrom
--help#2155Conversation
9716ad2 to
2953d29
Compare
2953d29 to
961919e
Compare
|
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:
|
The idiomatic way in Rust is to make invalid types unrepresentable instead of paranoidly calling a validate method everywhere.
5397553 to
468b31f
Compare
468b31f to
414227a
Compare
--help
|
I have updated this PR to only group the options in To be clear this PR is now purely a refactor (aside from the changed
Yes I'd expect it to always work. I think it makes sense that a tool defaults to its main functionality.
I think subcommands primarily make sense when they have a different synopsis, e.g:
Thanks for asking this question ... it made me realize that |
--help--help
414227a to
d238509
Compare
ruff_cli/src/args.rs
Outdated
| #[arg(long, conflicts_with = "config")] | ||
| pub isolated: bool, | ||
| #[clap(long, overrides_with("show_source"), hide = true)] | ||
| no_show_source: bool, |
There was a problem hiding this comment.
Should this not be grouped with show_source? Same for no_fix and fix.
There was a problem hiding this comment.
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.
d238509 to
d8ccfc1
Compare
| CARGO_PKG_VERSION | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Are you saying you don't like my code...? (Joking)
There was a problem hiding this comment.
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 ...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
This is the followup PR for #2145.
New
ruff --helpoutput: