Only complete appropriate arguments for typed commands.

[askreet]

Edit: This doesn't fix #2772, it does enable the fix. It fixes my issue that was closed as duplicate, though.

I think there's a bigger change we can make here to address some other code duplication that looks something like setting argument types of commands within the TypableCommand struct and using that data both for completion and for verification of the number of arguments. Currently, some commands have checks for argument length and emit errors while others silently ignore trailing arguments.

I'm not thrilled about having variadic length completers represented as different enum variants but as we statically allocate all the command configuration I couldn't use a dynamic container. I've also left the implementation of arity 2 completers for set-option, even though I haven't implemented a completer for option values (I've got another patch I'm working on that will dive into option metadata for #5939 and can tie that in).

Edit: I realized after reading my own PR that I didn't need different variants for variadic completers, but I do need lifetime annotations to use slices. I'm probably irrationally allergic to lifetime annotations, but this does seem a bit cleaner.

[pascalkuthe]

This is a nice UX Improvement and has potential for even netter followup improvements. I like that this is quite small in scope. The diff is only a bit larger because you and to.change the comlleteres for all typable commands 👍

[pascalkuthe]

I don't think an enum is an amazing fit here. NoComoletions is just a subset of positional and this approach doesn't allow commands which have a couple positional arguments followed by varags. Instead you should simply use a list of positional arguments followed by an optional vararg completed:

strict CommandSignature{
   postional_args: &'static [Option<Completer>],
   var_arcs: Option<Completer>
} 

You can get the compler for field n with:

self.positional_arts.get(n).unwrap_or(self.var_args)

Note that my suggestion uses a 'ststic slice. Tgis removes the lifetime argument you were unaopy with and is a good fit here because currently all tupable commands are static anyway and if we move away from that we need to heap allocate anyway for that cade a cow would be used)

[pascalkuthe]

Probably not in this this PR but in the future having an enum completer would be nice for cases like this. An autoamtic way to do that would also be useful for #4411 but I am not sure if serde allows that.

[askreet]

Yeah I want to have more metadata about options in general, some discussion in #5939.

[pascalkuthe]

Yeah that should be left to future work, just something that came to mind while reviewing

[askreet]

Thanks for the tips @pascalkuthe, pushed up some updates. I think your suggestion is much cleaner, especially using 'static scope.

[pascalkuthe]

I had a couple more ideas to make the API even more compact

[pascalkuthe]

Actually thinking about it more I don't think we need to use Option<Completer> here we can just use ui::completers::none instead.
This will allow creating some utility functions (see below) to make the API a bit cleaner.

[pascalkuthe]

I think adding a couple more utility constructs will go a long way towards keeping the command list short and readable.

Suggested change

	const fn none() -> Self {
	Self {
	positional_args: &[],
	var_args: None,
	}
	}
	const fn none() -> Self {
	Self {
	positional_args: &[],
	var_args: completers::none,
	}
	}
	const fn single(completer: &'static Completer) -> Self {
	Self {
	positional_args: slice::from_ref(completer),
	var_args: completers::none,
	}
	}
	const fn repeat(completer: Completer) -> Self {
	Self {
	positional_args: &[],
	var_args: completer,
	}
	}

[askreet]

You know it's funny I actually wrote basically the same thing first and wasn't sure if people would be comfortable with the layers of indirection. I agree and will put it back! :-)

[askreet]

@pascalkuthe Updates ready! :)

[pascalkuthe]

@pascalkuthe Updates ready! :)

Ah I actually mean that the positional_arguments should also be just a Completer instead of Option<Completer> would remove all the Some(..) wrapping (and you can always use completers:;none instead of None)

[askreet]

Ooh, yeah. Will do.

…

On Sat, Feb 18, 2023, 4:37 PM Pascal Kuthe ***@***.***> wrote: @pascalkuthe <https://github.com/pascalkuthe> Updates ready! :) Ah I actually mean that the positional_arguments should also be just a Completer instead of Option<Completer> would remove all the Some(..) wrapping (and you can always use completers:;none instead of None) — Reply to this email directly, view it on GitHub <#5966 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFB2KESYNMV6MOLEHE6NKDWYE6LJANCNFSM6AAAAAAU2I5EUE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[pascalkuthe]

LGTM now 👍