-
Notifications
You must be signed in to change notification settings - Fork 421
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
cli: default to log when no subcommand is provided #1525
Conversation
59a0696
to
0ea1511
Compare
Since the working copy is "committed" at some invocations of |
0ea1511
to
62e64e9
Compare
FYI, it's practically all.
It already does that... |
@joyously by the way, it might not be obvious from this PR, but it's changing jj to match Sapling's behavior, where it shows the graph log when you invoke |
I sort of got that from the comments. |
One hesitation I have is that I actually find the output of I find One option I can think of is to have
Another alternative I've been thinking about is encouraging Finally, we could look into making I don't think these worries have to prevent us from merging this and trying it out, but I thought I'd share them. |
62e64e9
to
bb0edb6
Compare
Okay, I've pushed a new version where the default command is resolved earlier in the process per @martinvonz's suggestion. That did solve the previous limitation of only being able to parse global options, and it will now pass through any options as expected. The behavior can be configured by setting The one minor issue I've seen with the approach is that specifying |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this feature is a bit weird because of the inconsistencies (like the jj -T show
case you mentioned).
I also think @ilyagr had a good point about showing the short help to new users, but maybe we can improve that by printing warning (as Ilya said).
bb0edb6
to
4cb52d1
Compare
I came up with a less-naive approach and now check the ArgMatches directly, so I believe all inconsistencies have been removed ( |
Related to the preference for short help, it might be possible, but it would take a bit of work. Clap's auto-generated |
Oh, and I think we can easily print out the above suggested hint if the setting isn't in place. Give me a bit and I'll push that change and tests. EDIT: done. |
bf4259d
to
17aeb6a
Compare
31d9817
to
d71d77f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty simple now :) Thanks!
This is a convenience optimization to improve the default user experience, since `jj log` is a frequently run command. Accessing the help information explicitly still follows normal CLI conventions, and instructions are displayed appropriately if the user happens to make a mistake. Discoverability should not be adversely harmed. Note that this behavior mirrors what Sapling does [2], where `sl` will display the smartlog by default. [1] clap-rs/clap#975 [2] https://sapling-scm.com/docs/overview/smartlog
d71d77f
to
b2924e8
Compare
This is a convenience optimization to improve the default user experience, since
jj log
is a frequently run command. Accessing the help information explicitly still follows normal CLI conventions, and instructions are displayed appropriately if the user happens to make a mistake. Discoverability should not be adversely harmed.Note that this behavior mirrors what Sapling does, where
sl
will display the smartlog by default.If people are onboard with this change, where else would you recommend that the behavior be documented?
Checklist
If applicable:
CHANGELOG.md