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

cli: bookmark move: allow short aliases for --to/--from #5943

Merged
merged 1 commit into from
Mar 10, 2025

Conversation

calebdw
Copy link
Member

@calebdw calebdw commented Mar 9, 2025

Hello!

If --to is going to become a required argument, it should have a short alias as it will be used quite frequently.

Additionally, I would argue in favor of using -f for --from, given that is it used for other commands and we should be consistent (if users can use -f as an alias for --from on other commands then logically they would assume they can use it for this one). Not allowing it because users might mistake it to mean --force isn't that big of a concern in my book given that --force is not used as an option anywhere else (as far as I am aware) and the option requires an argument so it will fail even if a user does try to use it for such a purpose.

If this is acceptable then I can send a follow up PR to add the alias for --from (or edit the current commit).

Thanks!

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have updated the config schema (cli/src/config-schema.json)
  • I have added tests to cover my changes

Sorry, something went wrong.

@ilyagr
Copy link
Contributor

ilyagr commented Mar 9, 2025

I'm the one who added those comments, and I'm not sure how strongly I feel about this today. I'll lay out the arguments that do come to mind, and I'm curious what others think about it.

I think we should either support neither -f nor -t, or we should support both. So, you should probably add -f to this PR as well. (Or, perhaps, this is a point we could discuss, given what I say below).

The fact that -f requires an argument helps. However, one issue that comes to mind is that jj bookmark move -f mybookmark will be interpreted as --from mybookmark even if the user wanted to use --force and thought of mybookmark as a "names" argument.

If we think this is a sufficient reason to reject this PR, we should probably add it to the comment.

BTW, as an aside, I was hoping that most people would end up typing --f<TAB>, which is 4 key presses (compared to -f being 2 and --force being 6; --f<TAB> also removes the need for pressing space afterwards if it's not the last argument). I think I often type out --from, though, and I don't mind it too much.

@yuja
Copy link
Contributor

yuja commented Mar 9, 2025

Just add both -f and -t? We already have a couple of places where -f means --from, and I don't think jj bookmark move is special.

jj bookmark -f name will be a hard error (if --to becomes mandatory.)

@calebdw calebdw force-pushed the push-nnnwzumvznpm branch from 2030ec2 to 2985809 Compare March 9, 2025 18:09
@calebdw calebdw changed the title cli: bookmark move: allow short alias for --to cli: bookmark move: allow short aliases for --to/--from Mar 9, 2025
@calebdw calebdw force-pushed the push-nnnwzumvznpm branch from 2985809 to 9ff1ca1 Compare March 9, 2025 18:12
If `--to` is going to become a required argument, it should
have a short alias as it will be used quite frequently.

Given that `--to` has a short alias it only makes sense to
allow `-f` for `--from` as this is consistent with other
commands and nothing makes this particular command special.
@calebdw calebdw force-pushed the push-nnnwzumvznpm branch from 9ff1ca1 to 52ae882 Compare March 9, 2025 18:13
@calebdw
Copy link
Member Author

calebdw commented Mar 9, 2025

I originally had added the -f alias, but I removed it because my primary concern was being able to quickly type -t for --to (yes I'm lazy 🙃) and I didn't want this PR being rejected for the -f. Given that --from is not required (and I'm not sure how often it is really used, personally I'm not sure when I would ever use it) I'm also okay with tab completing it. However, as there appears to be concurrence, I've re-added the -f alias.

I think we should either support neither -f nor -t, or we should support both.

While I agree that we should support -f, I would still like to support -t even if we decide not to go with -f.

We already have a couple of places where -f means --from, and I don't think jj bookmark move is special.

This is my biggest argument---nothing makes this particular command special and users are going to expect that they can use -f/-t for --from/--to as they can for other commands. Not supporting it is only going to cause additional issues and questions (what makes this command special, etc.). Consistency in aliases and argument names across the different commands is key.

Copy link
Contributor

@yuja yuja left a comment

Choose a reason for hiding this comment

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

Looks good, thanks.

@ilyagr
Copy link
Contributor

ilyagr commented Mar 10, 2025

For completeness, I'll post the unfinished draft of a response I was writing. Ultimately, I'm fine with you guys making a decision, both on --from and on whether it makes sense to have -t without -f. It sounds like Yuya is fine with the later, and I'm happy to defer to him on this


I'm still on the fence. At the moment, I continue to think that allowing -f does have the potential to cause minor confusion, but I'm not sure it's significant enough to outweigh the convenience of the shortcut.

IIRC, I was also concerned that jj bookmark move would often be used right before a jj git push, so if jj bookmark move does the wrong thing without the user noticing, they could make an error that's difficult to recover from.

jj bookmark -f name will be a hard error (if --to becomes mandatory.)

I don't think --to becoming mandatory would change much. The example would just be1 jj bookmark move --to @ -f mybookmark rather than jj bookmark move -f mybookmark.

Some possible confusion this would lead to:

  1. If there is another bookmark at the same revision as mybookmark, the user might not notice that the command moves both bookmarks. If they thought -f meant --force, they'd expect only mybookmark to move. I think we do print a warning when multiple bookmarks are moved, so if they notice that, it's less of a problem.

  2. If mybookmark is not an ancestor of @, the user might not understand why this failed. Hopefully, the error message will help in this case.

Footnotes

  1. Though, I guess, it's nice that jj b m -f --to @ mybookmark would be an error.

@yuja
Copy link
Contributor

yuja commented Mar 10, 2025

My stance is that jj bookmark move is nothing special. Since we've already accepted the risk of confusion, and we'll never add -f/--force to any other commands, I have no reason to reject this PR.

@ilyagr
Copy link
Contributor

ilyagr commented Mar 10, 2025

OK, let's go with that then.

@calebdw
Copy link
Member Author

calebdw commented Mar 10, 2025

Sounds good, thanks folks!

@calebdw calebdw added this pull request to the merge queue Mar 10, 2025
Merged via the queue into jj-vcs:main with commit 072af84 Mar 10, 2025
24 checks passed
@calebdw calebdw deleted the push-nnnwzumvznpm branch March 10, 2025 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants