Skip to content

Subcommand followups#2288

Merged
charliermarsh merged 3 commits intoastral-sh:mainfrom
not-my-profile:subcommand-followups
Jan 28, 2023
Merged

Subcommand followups#2288
charliermarsh merged 3 commits intoastral-sh:mainfrom
not-my-profile:subcommand-followups

Conversation

@not-my-profile
Copy link
Contributor

@not-my-profile not-my-profile commented Jan 28, 2023 •
edited
Loading

(See the commit messages for the reasoning.)

@not-my-profile not-my-profile mentioned this pull request Jan 28, 2023
Merged
@not-my-profile
Remove --check alias introduced in eda2be6
1fbfc98 
We do not want to support the --{subcommand} legacy format for new
subcommands ... only for subcommands that used this format previously.
@not-my-profile not-my-profile force-pushed the subcommand-followups branch 2 times, most recently from 0f30301 to ad5ccb8 Compare January 28, 2023 03:33
@charliermarsh
Copy link
Member

charliermarsh commented Jan 28, 2023

Are there any analogies we can draw to other tools, to help inform the user-facing API for the CLI?

E.g., rustup has rustup toolchain list. Should we have ruff rule list instead of just ruff rule?

@not-my-profile
Copy link
Contributor Author

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

Yes I think it's quite common for commands to default to listing when there's no more apt action, examples that come to mind:

  • git branch
  • mount
  • ip address, ip route (etc. all the ip subcommands)
  • getent <database> [key], e.g. getent hosts

ruff rule list would be ambiguous in case there's a rule named list (which we'll probably never have since it would be a very non-descriptive rule name but it's still nice to avoid such ambiguities).

@charliermarsh
Copy link
Member

charliermarsh commented Jan 28, 2023

Appreciate it. I don't plan to release tonight either way :)

@not-my-profile
fix: help text and env for --format option of explain subcommand
562b115 
The doc comment and the env attribute were copied by mistake.
@not-my-profile
Rename new explain subcommand to rule
b9f0a93 
We probably want to introduce multiple explain subcommands and
overloading `explain` to explain it all seems like a bad idea.
We may want to introduce a subcommand to explain config options and
config options may end up having the same name as their rules, e.g. the
current `banned-api` is both a rule name (although not yet exposed to
the user) and a config option.

The idea is:

* `ruff rule` lists all rules supported by ruff
* `ruff rule <code>` explains a specific rule
* `ruff linter` lists all linters supported by ruff
* `ruff linter <name>` lists all rules/options supported by a specific linter

(After this commit only the 2nd case is implemented.)
@not-my-profile
Copy link
Contributor Author

not-my-profile commented Jan 28, 2023

I just want to note that if we want to change explain to rule (I personally think it's a good idea) we should do that before releasing the subcommand refactor so that the release notes in BREAKING_CHANGES.md stay up to date.

To clarify my commit message: At some point in the future we may have:

ruff config banned-api to explain the banned-api config option

ruff rule banned-api to explain the banned-api rule

... if we only had explain it would have to report both, which would be confusing. And we also would have to introduce separate subcommands for listing linters, rules, config settings ... I think having a setup where one subcommand lists everything by default and shows an individual item with another argument makes more sense.

This was referenced Jan 28, 2023
Merged
Merged
@charliermarsh charliermarsh merged commit dd79ec2 into astral-sh:main Jan 28, 2023
@charliermarsh
Copy link
Member

charliermarsh commented Jan 28, 2023

@not-my-profile - anything you think is missing / preventing us from cutting a release, now that this is merged?

@not-my-profile
Copy link
Contributor Author

not-my-profile commented Jan 28, 2023

No, I don't see anything else blocking a new release :)

@not-my-profile not-my-profile mentioned this pull request Feb 11, 2023
Closed
@ntBre ntBre mentioned this pull request Mar 9, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@not-my-profile @charliermarsh