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

Unify alias and default command parsing #5160

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gtsiam
Copy link
Contributor

@gtsiam gtsiam commented Dec 20, 2024

I'm trying to pull config parsing out of the cli. But to do that, we need to untangle config parsing and argument parsing. This is part of that.

As a bonus, you can now specify single-comamnd aliases as a string.

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

@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 20, 2024

Also, do we really want to forbid the user from aliasing built-in commands?

@gtsiam gtsiam force-pushed the unify-alias-parsing branch 2 times, most recently from 84d0f6f to 5dd0e84 Compare December 20, 2024 12:08
@bnjmnt4n
Copy link
Member

I'm trying to pull config parsing out of the cli. But to do that, we need to untangle config parsing and argument parsing. This is part of that.

Any reason for this?

As a bonus, you can now specify aliases some of the simpler aliases/default commands as a space-delimited string.

This wouldn't correctly handle cases where one of the arguments is a quoted string with whitespace within, would it? I'm not sure if there's a good enough justification for this then.

@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 20, 2024

Any reason for this?

#3034 Comes to mind. But my understanding is that we want to make using the jj-lib actually reasonably doable anyway. Currently, if you look at run_internal, you'll see that it's a mess of parsing the command line arguments, then the config then args again then re-parsing with a slightly different config etc, etc.

This wouldn't correctly handle cases where one of the arguments is a quoted string with whitespace within, would it? I'm not sure if there's a good enough justification for this then.

Not it wouldn't, and intentionally so. I don't want to implement a shell here. This is just a convenience thing that I've found myself personally wanting. If you want argument parsing, there's always the full array syntax. The alternative would be to treat single strings as single arguments, but that would go counter to what I would expect. (Also, see what git does).

That said, we could forbid quotes (", ' and `) to prevent this particular footgun. In fact I'm gonna do that right now.

cli/src/cli_util.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@yuja
Copy link
Contributor

yuja commented Dec 20, 2024

cc @matts1 as he was working on #3673

@gtsiam gtsiam marked this pull request as draft December 20, 2024 12:52
@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 20, 2024

I'll remove whitespace splitting and restore the old behaviour for now

@gtsiam gtsiam force-pushed the unify-alias-parsing branch 3 times, most recently from 37917a3 to 2be3011 Compare December 20, 2024 13:18
@gtsiam gtsiam requested a review from yuja December 20, 2024 13:18
@gtsiam gtsiam marked this pull request as ready for review December 20, 2024 13:19
@gtsiam
Copy link
Contributor Author

gtsiam commented Dec 20, 2024

Ok, this is just the refactor now. I tried to do too much in one pr I think.

Only real change now should be that you can specify single-command (no argument) aliases with a string. This is already possible for default-command, but now it is also possible in aliases.

Copy link
Member

Choose a reason for hiding this comment

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

cli: simplify argument expansion code

Could you add some more detail? This patch adds 39 lines of code, so it's not an obvious simplification. It would be good to know why you're doing it (if it's not only to make the code simpler). Since it's a non-trivial patch, it's also good to explain how you're refactoring the code so the readers don't have to spend so much time to try to figure out what's going on.

@gtsiam gtsiam force-pushed the unify-alias-parsing branch from 2be3011 to 488601c Compare December 24, 2024 20:03
@gtsiam gtsiam force-pushed the unify-alias-parsing branch from 488601c to 05a470a Compare December 24, 2024 20:05
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