RFC for an argument completer base class

[powercode]

No description provided.

[SteveL-MSFT]

What if the user is using a single quote?

[vexx32]

Oh gosh, had some flashbacks of working on argument completion for PSWordCloud. There's a bit more you need to handle for this kind of thing as well, depending on the colorful variety of things you may be expecting to handle as the completion results themselves. Let me find some code I wrote a while back... here:

https://github.com/vexx32/PSWordCloud/blob/main/Module/src/PSWordCloud/Completers/FontFamilyCompleter.cs

This one covers escaping things that need escaping, spaces, # characters in the completion results (I happened to end up having fonts with that character on my computer and it breaks completions terribly 😀) and probably a few other things. I haven't managed to break it yet.

The only thing I don't think it bothers to handle is whether the user used single quotes, it currently replaces them with double quotes instead I think. So it could be expanded on to detect that and act accordingly (less things need escaping there, etc).

[powercode]

I expect we have to turn this into a much more complete implementation. Maybe we can make use of the parser to determine what needs to be escaped?

[SteveL-MSFT]

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.

[vexx32]

@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.

[iSazonov]

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

[powercode]

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]

I have updated the RFC with more robust quoting

[rjmholt]

I think making a base class available to deduplicate functionality is a good idea.

I suspect us trying to perfect the implementation in an RFC is going to miss the forest for the trees.

I wouldn't have necessarily said an RFC was needed for this, but given that we are discussing this in an RFC the parts I'd want us to agree on here are:

• The name
• The API and exposed members and what their contracts are
• Possible future needs or directions for the class and how it might fit into the greater completion framework

[bergmeister]

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.

[powercode]

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