PowerShell-RFC icon indicating copy to clipboard operation
PowerShell-RFC copied to clipboard

RFC for an argument completer base class

Open powercode opened this issue 5 years ago • 9 comments

powercode avatar Nov 25 '20 16:11 powercode

I like the idea of a base class to remove the need to have common code for every completer as I've had to write it for my own completers. One consideration is whether it makes sense to have this as an independent nuget assembly so it can be used for different versions of PS rather than the one it gets checked into.

SteveL-MSFT avatar May 18 '21 19:05 SteveL-MSFT

@SteveL-MSFT I like the idea of that, but I think it would open up too many possible dependency conflicts. If you have two modules that each depend on a different version of that completion assembly, you can easily end up only being able to load one of them into a session at a time.

Until there's a way to isolate dependencies like that properly I think it's best left in the core pwsh toolkit, and folks can simply declare a dependency on a minimum PS version as usual without much hassle.

vexx32 avatar May 19 '21 12:05 vexx32

I see a point to put an end solution in separate nuget but it looks weird for base class.

iSazonov avatar May 19 '21 12:05 iSazonov

There already is an function in CompletionCompeters

private static bool CompletionRequiresQuotes(string completion, bool escape)
{
    // If the tokenizer sees the completion as more than two tokens, or if there is some error, then
    // some form of quoting is necessary (if it's a variable, we'd need ${}, filenames would need [], etc.)

    Language.Token[] tokens;
    ParseError[] errors;
    Language.Parser.ParseInput(completion, out tokens, out errors);

    char[] charToCheck = escape ? new[] { '$', '[', ']', '`' } : new[] { '$', '`' };

    // Expect no errors and 2 tokens (1 is for our completion, the other is eof)
    // Or if the completion is a keyword, we ignore the errors
    bool requireQuote = !(errors.Length == 0 && tokens.Length == 2);
    if ((!requireQuote && tokens[0] is StringToken) ||
        (tokens.Length == 2 && (tokens[0].TokenFlags & TokenFlags.Keyword) != 0))
    {
        requireQuote = false;
        var value = tokens[0].Text;
        if (value.IndexOfAny(charToCheck) != -1)
            requireQuote = true;
    }

    return requireQuote;
}

This could be reused. The second parameter is named somewhat unfortunate: escape should probably be something like isGlobbingPath.

powercode avatar May 20 '21 22:05 powercode

I have updated the RFC with more robust quoting

powercode avatar May 20 '21 23:05 powercode

The DevEX working group briefly discussed this and we think that the implementation doesn't necessarily have to be a base class, which could lead to other problems like this: https://en.wikipedia.org/wiki/Fragile_base_class Another alternative might be to expose an interface with extension methods or a mix of 'normal' methods and extension methods. @powercode If you are still motivated to go ahead, I suggest to follow up on @rjmholt previous comment and from then on I think should be easier to get the ball rolling.

bergmeister avatar Sep 21 '21 21:09 bergmeister

I'll tinker a bit with it and will get back with an update.

powercode avatar Sep 22 '21 12:09 powercode