Support parameterized argument completers

Add support for ArgumentCompletionAttributes with parameters

ArgumentCompleters are in the current implementation created by type, using a default constructor.

This makes it very hard to provide parameters to the completed, limiting their usefulness.

I propose that we add "Factory" support to the completion system, so that an attribute derived from ArgumentCompleterFactoryAttribute gets called on a method with a signature like

IArgumentCompleter Create();

This would allow the derived attribute to use attribute parameters to create the actual argument completer.

Deriving from ArgumentCompleterFactoryAttribute makes it possible to create generic completers, like

[DirectoryCompleter(ContainingFile="pswh.exe", Depth=2)]

[DateCompleter(WeekDay='Monday', From="LastYear")]

[GitCommits(Branch='release')]

An sample usage could look like this:

   /// <summary>
    /// Creates new number completions
    /// </summary>
    public class NumberCompletionsAttribute : ArgumentCompleterFactoryAttribute
    {
        private readonly int _from;
        private readonly int _to;
        private readonly int _step;

        public NumberCompletionsAttribute(int from, int to, int step = 1) 
        {
            _from = @from;
            _to = to;
            _step = step;
        }

        public IArgumentCompleter Create() => new NumberCompleter(_from, _to, _step);
    }

 public class NumberCompleter : IArgumentCompleter
    private readonly int _from;
    private readonly int _to;
    private readonly int _step;

    public NumberCompleter(int from, int to, int step)
    {
        _from = @from;
        _to = to;
        _step = step;
    }

    public IEnumerable<CompletionResult> CompleteArgument(string commandName, string parameterName, string wordToComplete, CommandAst commandAst, IDictionary fakeBoundParameters)
        {
/// complete using the passed parameters
        }
    }

Proposed technical implementation details (optional)

There are a couble of ways to go about this:

New abstract class ArgumentCompleterFactoryAttribute with an abstract Create method.
New interface IArgumentCompleterFactory That the attributes implement if they want the factory support.
Combine the above: Have ArgumentCompleterFactoryAttribute implement IArgumentCompleterFactory

Pull request for implementation can be found here: #12605

Dongbo Wang (@daxian-dbw)

From Dongbo Wang (@daxian-dbw)
I like the idea, very interesting, allowing people to have reusable argument completer attributes. About the implementation, why not have an abstract attribute class that defines an abstract method which returns an IArgumentCompleter instance? For example:

[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public abstract class ReusableArgumentCompletionAttribute : Attribute
{
    protected ReusableArgumentCompletionAttribute() {}
    public abstract IArgumentCompleter NewArgumentCompleter(/* EngineIntrinsics engineIntrinsics ?? */);
}

This seems more succinct than : ArgumentCompleterAttribute, IArgumentCompleterFactory

Another thought is IArgumentCompleter<T> vs IArgumentCompleterFactory so that users could use IArgumentCompleter, IArgumentCompleter<T>

Joey Aiello (@joeyaiello)

/cc Joey Aiello (@joeyaiello) Steve Lee (@SteveL-MSFT) Dongbo Wang (@daxian-dbw) James Truher (@JamesWTruher)

Dongbo Wang (@daxian-dbw)

The reason I have been leaning towards an interface is that there may turn up other places where we want to support creation of completers.

This can be combined with Dongbo Wang (@daxian-dbw)'s idea.

    [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
    public abstract class ArgumentCompleterFactoryAttribute : ArgumentCompleterAttribute, IArgumentCompleterFactory
    {
        // implements IArgumentCompleterFactory
        public abstract IArgumentCompleter Create();
    }

Users can still just derive from ArgumentCompleterFactoryAttribute but we maintain the flexibility of an interface.

On the other hand, we could probably just add the interface later if we see other useful scenarios.

Staffan Gustafsson (@powercode)

Staffan Gustafsson (@powercode) thanks for starting this conversation. Once the community has an agreement, the PS-Committee can review.

Maybe I'm just missing the point here, but I don't really see what this implementation offers that makes it easier / more robust than just creating an IArgumentCompleter class?

From the examples, it seems kind of like I'd now need to create two classes for every argument completer I want to create instead of just the one, and I feel like that's more hassle than it's worth in many ways. 😕

The whole point is to be able to create completers with parameters. The current mechanics only allow for a type, that is instantiated with with it's default constructor. You have no way of creating generic completers where the user can control their behavior on a case by case basis.

Think

param(
[FileCompleter(Extension=".json")]
[string]$Path
,
[FileCompleter(Extension=".txt", BaseNameFromParameter="Path")]
[string] $OutputPath
)

This would allow for the creation of libraries of completers, that can be implemented with high quality and used by others.

OK, I think that makes sense. I'm just not sure why we'd build it in such a way that requires a script author to implement two classes for it. Is it not possible to build this into a single class design that implements a given interface? I like that idea, I just think we might be overcomplicating the UX a bit here. 🙂

Personally, I envision the completers shipping in-box, or has libraries written in C#. The UX then would be to use the provided attributes, as demonstrated above

That makes sense for some of them, absolutely. I don't see why we wouldn't make them something users can easily build upon as well, though.

I have a concern about two classes too. We cannot completely decouple them.

Alternative could be public ArgumentCompleterAttribute(params object[] args) where first parameter is a custom Completer type and follows are its arguments.

The whole point is to be able to create completers with parameters. The current mechanics only allow for a type, that is instantiated with with it's default constructor. You have no way of creating generic completers where the user can control their behavior on a case by case basis.

It comes from the slightly strange way that completers are loaded.
For a validator I can write this

class ValidatePathAttribute :  System.Management.Automation.ValidateArgumentsAttribute {
    [string]$exception = "" 
    [boolean]$ContainersOnly = $false
    [int]$MaxItems = -1 
    [void] Validate(
etc

and use
[ValidatePath(exception="^-+$",ContainersOnly=$true,MaxItems=1)]
in one place (so ^, - + and $ will be treated as valid and it must resolve to 1 container)
and somewhere else I can use it without setting anything to validate files or folders and as many matches as you like

But with a completer because the specification is
[ArgumentCompleter([PathCompleter])]

there is no option to say "Create the instance of PathCompleter with these parameters set before you call completeArgument()"
There is a work round by writing argumentCompleter({scriptblock}) but it makes for an ugly and hard to read script block, so two similar completers must be duplicate most of their code because they parameterize a choice like "expand files and directories" vs "expand only directories".

public ArgumentCompleterAttribute(params object[] args) where first parameter is a custom Completer type and follows are its arguments.

would be the most sensible way to add in the functionality.

I almost think we'd be better off adding a side-by-side implementation of an argument completer interface that works more like the validation attributes would be better. 🤷

Dunno about that last point on the implementation detail, feels like there's probably a better way to do that. At the very least, public ArgumentCompleterAttribute(IArgumentCompleter completer, params object[] args) seems a better bet. 🙂

Argument completer is a special case (AIUI) because there can be as many transformers and validators as required, but there can't be two completers for the same thing trying to send lists back. So we have only one argument completing class with a constructor which takes either a class (with specific requirements) or a script block (with specific requirements). Changing that would probably have complicated ripple effects.

But provided there is a way of writing
[Here_Be_Completion <<class name>> <<attribute values>> ] I don't think many people will care very much about the details of the syntax, or what it looks like internally.

I think you are missing an important point here.
Using properties have a documenting effect when using attributes. Using a params array in construction makes it impossible to use an attribute completer in more that one way, and it gives absolutely no information to the user what arguments to pass.

I think you have to come to terms with the fact that the attribute and the completer are two different things.

We could create a completer for ArgumentCompleterAttribute(type_completer, <Tab> but I think it's not even worth the effort - if the user knows about the existence of this type_completer completer, then he knows how to use it or he is already looking at its description in the documentation.

I really think we should drop the params argument idea.

This is what the users would see

[ArgumentCompleter([GitCommits], $true, $false, 15, 'release')]

What does that mean?

The properties of the attributes have a documenting effect.

[GitCompleter(SkipMerges=$true, IncludeCurrentUser=$false,  OnlyUsersWithMoreCommitsThan=15, Branch='release')]

[ArgumentCompleter([GitCommits], a=$true, b=$false, c=15, d='release')]$var

is parsed well.

Staffan Gustafsson (@powercode)

It parses, but the named arguments would have to map to properties on [ArgumentCompleter] which is why I'd think it's more useful to make ArgumentCompleter effectively inheritable or something along those lines so we can have the syntax Staffan Gustafsson (@powercode) is suggesting with more than just the initial set of attributes he proposed using, without complicating the design more than is needed.

but the named arguments would have to map

Can't PowerShell know such bindings? :-)

@PowerShell/powershell-committee reviewed this. We agree that the current proposed design is the preferred one as the ability to have named parameters makes the code more understandable. There was a general concern about introducing a new attribute which means modules require a specific version of an assembly (SMA.dll if these completers are part of PS or a shared assembly from nuget.org, for example), but this is true for any new compiled feature in PS. Overall, we thought this was a good discussion and brought up many good points. However, to make it easier to review and understand the user experience better, it would be great to have more example code in the issue. In this case, the example code was in the PR as part of the test case:

class NumberCompleter : IArgumentCompleter 
{

    [int] $From
    [int] $To
    [int] $Step

    NumberCompleter([int] $from, [int] $to, [int] $step) 
    {
        if ($from -gt $to) {
            throw [ArgumentOutOfRangeException]::new("from")
        }
        $this.From = $from
        $this.To = $to
        $this.Step = if($step -lt 1) { 1 } else { $step }
    }

    [IEnumerable[CompletionResult]] CompleteArgument(
        [string] $CommandName,
        [string] $parameterName,
        [string] $wordToComplete,
        [CommandAst] $commandAst,
        [IDictionary] $fakeBoundParameters)
    {
        $resultList = [List[CompletionResult]]::new()
        $local:to = $this.To
        for ($i = $this.From; $i -le $to; $i += $this.Step) {
            if ($i.ToString().StartsWith($wordToComplete, [System.StringComparison]::Ordinal)) {
                $num = $i.ToString()
                $resultList.Add([CompletionResult]::new($num, $num, "ParameterValue", $num))
            }
        }

        return $resultList
    }
}

class NumberCompletionAttribute : ArgumentCompleterAttribute, IArgumentCompleterFactory
{
    [int] $From
    [int] $To
    [int] $Step

    NumberCompletionAttribute([int] $from, [int] $to)
    {
        $this.From = $from
        $this.To = $to
        $this.Step = 1
    }

    [IArgumentCompleter] Create() { return [NumberCompleter]::new($this.From, $this.To, $this.Step) }
}

function FactoryCompletionAdd {
    param(
        [NumberCompletion(0, 50, Step = 5)]
        [int] $Number
    )
}

I will probably become an issue of documentation, since I envision the box product to contain completers, just as it has validation attributes. Generic completers for files/directories, AD, etc will eventually become available and needs to be documented just like the attributes.

Navigation Menu

Repository navigation

Support parameterized argument completers #12708

Add support for ArgumentCompletionAttributes with parameters

Proposed technical implementation details (optional)

1 remaining item

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Support parameterized argument completers #12708

Description

Add support for ArgumentCompletionAttributes with parameters

Proposed technical implementation details (optional)

Activity

iSazonov commented on May 18, 2020

iSazonov commented on May 18, 2020

iSazonov commented on May 18, 2020

powercode commented on May 18, 2020

SteveL-MSFT commented on May 20, 2020

vexx32 commented on May 20, 2020

powercode commented on May 20, 2020

vexx32 commented on May 20, 2020

powercode commented on May 20, 2020

vexx32 commented on May 20, 2020

iSazonov commented on May 21, 2020

jhoneill commented on May 21, 2020

1 remaining item

jhoneill commented on May 22, 2020

powercode commented on May 22, 2020

iSazonov commented on May 22, 2020

powercode commented on May 22, 2020

iSazonov commented on May 22, 2020

vexx32 commented on May 22, 2020

iSazonov commented on May 22, 2020

SteveL-MSFT commented on Jun 18, 2020

powercode commented on Jun 22, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions