Support parameterized argument completers #12708
Comments
|
From @daxian-dbw [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public abstract class ReusableArgumentCompletionAttribute : Attribute
{
protected ReusableArgumentCompletionAttribute() {}
public abstract IArgumentCompleter NewArgumentCompleter(/* EngineIntrinsics engineIntrinsics ?? */);
}This seems more succinct than |
|
Another thought is IArgumentCompleter<T> vs IArgumentCompleterFactory so that users could use |
|
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 @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 On the other hand, we could probably just add the interface later if we see other useful scenarios. |
|
@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 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 |
It comes from the slightly strange way that completers are loaded. and use But with a completer because the specification is there is no option to say "Create the instance of PathCompleter with these parameters set before you call completeArgument()"
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, |
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 |
|
I think you are missing an important point here. 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 |
|
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')]$varis parsed well. |
|
It parses, but the named arguments would have to map to properties on |
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. |
|
🎉This issue was addressed in #12605, which has now been successfully released as Handy links: |
Uh oh!
There was an error while loading. Please reload this page.
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
ArgumentCompleterFactoryAttributegets called on a method with a signature likeThis 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
An sample usage could look like this:
Proposed technical implementation details (optional)
There are a couble of ways to go about this:
ArgumentCompleterFactoryAttributewith an abstractCreatemethod.IArgumentCompleterFactoryThat the attributes implement if they want the factory support.ArgumentCompleterFactoryAttributeimplementIArgumentCompleterFactoryPull request for implementation can be found here: #12605
The text was updated successfully, but these errors were encountered: