MartinGC94 / UsefulArgumentCompleters

MIT License
9 stars 1 forks source link

Dealing with conflicting argument completers #3

Open jdhitsolutions opened 2 years ago

jdhitsolutions commented 2 years ago

Currently, it appears that most argument completers are loaded automatically when importing the module. That may work fine for you. But there is the potential for conflict. What if I have my own completer for Get-WinEvent that I prefer. Importing this module overwrites it. Have you considered a more granular approach? Maybe don't automatically import completers. Let the user decide by command or module. This is related to Issue #2 . There is terrific value in this module and I want to see more people use it, but I think you need to address useabiltiy.

MartinGC94 commented 2 years ago

Realistically the only reason not to import an argument completer is if it's conflicting with an existing one (like you said) but if someone has a better argument completer for Get-WinEvent then I would prefer they contribute it to this module instead of keeping it to themselves.
If it's an argument completer that is specific to their environment that doesn't make sense for the general public then they can easily override an argument completer from this module by simply defining their argument computer after this module gets imported, eg having a $profile that looks like this:

Import-Module UsefulArgumentCompleters
Register-ArgumentCompleter -CommandName Get-WinEvent...

If this was a completely free feature to add then I would have no problem adding it but I'm afraid it would hurt the import time of the module if it has to (potentially) make 450+ Register-ArgumentCompleter calls + some logic instead of the ≈ 50 calls it's currently making to register them all. PowerShell is already really slow to launch so every millisecond counts IMO.

jdhitsolutions commented 2 years ago

I'm confused. If anything, my suggestion improves performance since the user only imports what they need. Importing the module shouldn't load the argument completers. Let the user do it via the import command. Yes, this would be a breaking change but one for the better.

Another option is to let the user store a list of argument completers, say in $home. If the file exists on module import, then automatically load those argument completers.

I realize you probably developed this module for the way you work. But we, as PowerShell toolmakers, need to consider how our tools will be used and the user's expectations. That is why I'm giving you this feedback.

MartinGC94 commented 2 years ago

What I mean is that each command invocation has a cost so when I need to run Register-ArgumentCompleter it's better to group as many commands into one call as possible instead of running it for every individual command. If I had to make a function that lets the user specify which completers to load it would probably look something like this:

foreach ($Completer in $AllCompleters)
{
    if ($Completer.Name -like $UserInput)
    {
        Register-ArgumentCompleter $Completer
    }
}

and I would lose the ability to group them together for that efficiency.
I did a simple prototype on my own PC where I split all the command arrays into individual calls to Register-ArgumentCompleter and the import time increased from 0.109 to 0.145 which isn't as much as I had feared but it's still an undesirable increase. It's true that you may gain some performance by limiting the completers but I think you would need to cut a lot to make up for the lost performance.

36ms isn't a lot, I could live with that if it made the overall experience better but I just don't see any noteworthy benefit from moving it to a command.
As for user expectations, it's true that many modules use commands to export their data but it's certainly not the only way. A module can exclusively consist of classes that the user is expected to use with using module XXX. Modules like the VMware PowerCLI modules can make a PSProvider available on import. PSReadline and posh-git changes the console experience on import.

jdhitsolutions commented 2 years ago

I had to look at your code more closely, and now I have a better understanding. I can see how importing a specific argument completer would be problematic without a major overhaul of the module. But you are already grouping completers, so it wouldn't be that hard to move the code from the psm1 file to the import function. You've already started it with the OptionalCompleter parameter. You just need to tag your sets, my preference is by module name, and enhance what you've already started.

MartinGC94 commented 2 years ago

So, like I said before I personally don't see a lot of value in doing this so I'm not going to make this change but I'm not completely against the idea.
If you or someone else wants to do this then I would accept a PR as long as the following 2 conditions are met:
1: It doesn't make development of new completers harder (I don't want to do something like tagging individual completers but I can live with organizing them in special folders).
2: Import performance when importing all the completers should not be noticeably affected. The additional 36 ms I measured before was basically a worst case scenario so I would expect a less granular approach to be faster but even if it wasn't I would accept something in that range (but not much more than that).