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: move jj co alias to config file, so it can be overridden #3242

Merged
merged 1 commit into from
Mar 7, 2024

Conversation

martinvonz
Copy link
Member

This way users can override jj co to mean jj new if they want to get rid of the warning.

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

This way users can override `jj co` to mean `jj new` if they want to
get rid of the warning.
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.

Thanks!

@martinvonz
Copy link
Member Author

@thoughtpolice, @PhilipMetzger: are you okay with this?

Copy link
Contributor

@thoughtpolice thoughtpolice left a comment

Choose a reason for hiding this comment

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

Yeah, LGTM. I should have done this in the original PR, honestly, since I wanted to free up these names as early as possible so people could reuse co. Thanks!

Copy link
Contributor

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

LG. I think for the future, we should just provide the aliases as built-in config, instead of defining them via clap. Then everyone is free to define them as is.

@martinvonz martinvonz merged commit 663e352 into main Mar 7, 2024
16 checks passed
@martinvonz martinvonz deleted the push-wwqsqtkqqmtw branch March 7, 2024 17:46
@martinvonz
Copy link
Member Author

I think for the future, we should just provide the aliases as built-in config, instead of defining them via clap.

I hesitate to do that for two reasons:

  • Aliases configured in clap could show up in the help text, though it seems that clap doesn't currently do that (it does for flag aliases)
  • By having a builtin alias like jj undo, we can refer to that in hints and such. Maybe it's still fine to do that and just tell people who define aliases.undo = ["new"] to not shoot themselves in the foot

@PhilipMetzger
Copy link
Contributor

PhilipMetzger commented Mar 7, 2024

I don't mind having a set of core aliases built-in but convenience aliases should be defined in the config.

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.

4 participants