-
Notifications
You must be signed in to change notification settings - Fork 349
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
feat: Add support for *args to AliasMap #4605
base: main
Are you sure you want to change the base?
Conversation
This is required in order to support command aliases as functions, because they will require the following to be supported: ```toml 'upload()' = ["upload", "@"] 'upload(r)' = [["fix", "$r"], ["git", "push", "--change", "$r"]] ``` Here, `jj upload` would need to map to the first, `jj upload foo` would need to map to the second, and `jj upload foo --bar` would also need to map to the second.
These examples don't look like Perhaps, we'll need a public API to get let overloads = map.get_function_overloads("upload")?;
// for `jj upload foo --bar`, max_parameters = ["foo"].len()
// XXX should we pick upload/1 for `jj upload foo bar`?? It might be rather confusing.
let arity = min(overloads.max_arity(), max_parameters);
// If there were upload/0, /1, /3, and max_parameters is 2, I think it's better
// to error out (as ambiguous alias definitions) than falling back to upload/1.
let function = overloads.find_by_arity(arity)?; |
Oh, but |
just a minor nit from my side, "feature" is not a known topic, it could be |
My original intention was that every overload would have an implicit
My logic was going to be:
Your proposal does work as well, I mostly wanted to avoid the
This would generate an |
My feeling is that choosing a prefix match would lead to random unexpected result. I would rather want to see an error if overloads set is somewhat inconsistent. BTW, the example above has recursion?
I don't have a strong feeling about whether |
This is required in order to support command aliases as functions, because they will require the following to be supported:
Here,
jj upload
would need to map to the first,jj upload foo
would need to map to the second, andjj upload foo --bar
would also need to map to the second.Checklist
If applicable:
CHANGELOG.md