Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 2279 #2282

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Issue 2279 #2282

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e023a78
Fix issue 2279
Oct 26, 2023
c3c1333
#2279 - add test
Oct 26, 2023
7a87dc9
2279 - remove unused declarations
Oct 26, 2023
43d2c18
#2279 - fix test
Oct 29, 2023
1847bcc
2279 - fix completioncontext issue 2279
Oct 31, 2023
4d8d7e8
add some tests - 2279
Oct 31, 2023
abe31f9
2279 - fix bug where = sign could not be used in a CLi option value
Nov 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/System.CommandLine.Tests/CompletionContextTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,32 @@ public void CommandLineText_preserves_command_line_prior_to_splitting_when_compl
.Be(commandLine);
}

[Fact]
public void CommandLineText_is_parsed_when_option_is_in_name_equals_sign_value_format()
{
CliRootCommand command = new CliRootCommand
{
new CliCommand("inner")
{
new CliOption<string>("--optionOne"),
new CliOption<string>("--optionTwo")
}
};

var commandLine = "outer inner --optionOne argument1 --optionTwo=argument2";

var parseResult = command.Parse(commandLine);

parseResult.GetCompletionContext()
.Should()
.BeOfType<TextCompletionContext>()
.Which
.CommandLineText
.Should()
.Be(commandLine);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this new test demonstrate the bug, i.e. fail if the bug is not fixed? The call stack of the exception in the issue included ParseResult.GetCompletions, but this test does not seem to call that function.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive checked and surprisingly - no it does not.

when i manually run the following code manually ( with or without a debugger ) it throws an error before I apply my fix ( only if the argument has a = symbol separating the key value ) and after I apply my fix it works. - IE exactly what it should do.

` CliRootCommand _outerCommand = new CliRootCommand
{
new CliCommand("inner")
{
new CliOption("--optionOne"),
new CliOption("--optionTwo")
}
};

    var result = _outerCommand.Parse("ou\\=ter inner --optionOne argument1 --optionTwo=argument2");
    result.GetCompletions();

`

Within my test I have almost exactly the same code - the only differences are _outerCommand is now named command and the command line string is now defined in a variable before the command is called.
neither of these issues should have changed it.

However when i reverted my fix the test still worked.

I tried adding a check to detect the 'InvalidOperationException' and it made no difference

I tried the following test code and its not failing without my fix being applied ( note i added the manual "throw new InvalidOperationException("hello");" to prove that my test is correctly detecting errors, I also ran it without this line )

` public void CommandLineText_is_parsed_when_option_is_in_name_equals_sign_value_format()
{

        Assert.Throws<InvalidOperationException>(methodThatThrows);
            
        void methodThatThrows()
        {
            CliRootCommand command = new CliRootCommand
            {
                new CliCommand("inner")
                {
                    new CliOption<string>("--optionOne"),
                    new CliOption<string>("--optionTwo")
                }
            };

            var commandLine = "outer inner --optionOne argument1 --optionTwo=argument2";

            var parseResult = command.Parse(commandLine);

            throw new InvalidOperationException("hello");

        }
            
    }`

I dont know what to do here or why my functionally identical code is behaving differently when it runs in test.
Advice would be appreciated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the argument of GetCompletions makes a difference. In #2279 (comment),

(Note: I found the easiest way to actually reproduce this was by running this with the arguments of: [suggest:30] "CompletionCrashRepro --test=asd" rather than actually doing tab completion.)

which would presumably call GetCompletions(30). Your code above calls result.GetCompletions() instead.

The length of CompletionCrashRepro --test=asd is 31 characters. I think 30 then means the insertion point would have been between --test=as and d.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also
i tried adding 'System.Environment.Exit(2);' to 'GetWordToComplete' in 'CompletionContext.cs' and running the test and non of them fail which shows that none of the tests run this piece of code.

They do exit when running the code manually.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest you run the test in a debugger and trace why it doesn't go to GetWordToComplete.

It might be related to how the CliRootCommand constructor uses CliRootCommand.ExecutableName as the name of the command. CliRootCommand derives that from Environment.GetCommandLineArgs()[0], which would differ between the test runner process and your experimental application.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test fixed - its working properly now.
please finish reviwing


}

[Fact]
public void CommandLineText_is_preserved_when_adjusting_position()
{
Expand Down
4 changes: 2 additions & 2 deletions src/System.CommandLine/Completions/CompletionContext.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,8 @@ protected static string GetWordToComplete(

var textAfterCursor = rawInput.Substring(position.Value);

return textBeforeCursor.Split(' ').LastOrDefault() +
textAfterCursor.Split(' ').FirstOrDefault();
return textBeforeCursor.Split(' ','=').LastOrDefault() +
textAfterCursor.Split(' ','=').FirstOrDefault();
}

return "";
Expand Down