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.

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

Fix #1385 #1390

Merged
merged 2 commits into from
Aug 28, 2021
Merged

Fix #1385 #1390

merged 2 commits into from
Aug 28, 2021

Conversation

krsahab
Copy link
Contributor

@krsahab krsahab commented Aug 27, 2021

Fix for issue #1385
Ignore whitespace inside the quote
Ignore quotes if seeking == Boundary.WordEnd
Tests added

Fix for issue #1385
Ignore whitespace inside the quote
Ignore quotes if seeking == Boundary.WordEnd
Handle edge cases
@krsahab krsahab mentioned this pull request Aug 27, 2021
@@ -25,52 +27,66 @@ public IEnumerable<string> Split(string commandLine)
var pos = 0;

var seeking = Boundary.TokenStart;
int? skipQuoteAtIndex = null;
var seekingQuote = Boundary.QuoteStart;
StringBuilder sb = new StringBuilder();
Copy link
Contributor

Choose a reason for hiding this comment

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

Perf challenge #2: Can you do it without the StringBuilder?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

StringBuilder removed and code modified accordingly

Copy link
Contributor

Choose a reason for hiding this comment

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

Beautiful! Nice work!

Optimizing string operations
@jonsequitur
Copy link
Contributor

Thanks, @krsahab!

@jonsequitur jonsequitur merged commit 534eeac into dotnet:main Aug 28, 2021
@krsahab krsahab deleted the fix-#1385 branch August 29, 2021 04:46

_splitter.Split(commandLine)
.Should()
.BeEquivalentTo("POST", "--raw='{Id:1,Name:Alice}'");
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. The double quotes should be preserved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Minimal Changes have been done just to fix the mentioned issue and not to modify any existing logic.
It was preexisting logic to stripe away all the double-quotes. I tried to preserve it but it was conflicting with other test cases.

@database64128
Copy link
Contributor

In scenarios such as a simple REPL (https://github.com/database64128/DotnetPlayground/blob/main/CommandLinePlayground/Program.cs#L35), one would expect that each argument was parsed as is. Currently all double quotes are striped away. An input string such as --plugin '{"validJsonFields":"RequireDoubleQuotes"}' would become '{validJsonFields:RequireDoubleQuotes}'. Thie single quotes should have been striped away. The double quotes should have been preserved.

@krsahab
Copy link
Contributor Author

krsahab commented Aug 29, 2021

As per preexisting logic, only double quotes should be removed, not any other characters (whitespace is a special case). If single quotes should be removed, then the question is What all characters should be removed or preserved?

@database64128
Copy link
Contributor

@krsahab Thank you for the explanation! Maybe I should open a new issue for my proposed changes.

If single quotes should be removed, then the question is What all characters should be removed or preserved?

I think the behavior should be aligned with popular shells: Remove the outermost pair of quotes.

@jonsequitur
Copy link
Contributor

The behavior is intended to match this: https://docs.microsoft.com/en-us/windows/win32/api/shellapi/nf-shellapi-commandlinetoargvw. I'm sure we've missed some details.

This is separate from the shell behavior. Different shells have their own escaping rules, so in many cases the application entry point might never even see the quotes you typed into the terminal. CommandLineToArgvW then has additional behaviors that will strip out quotes. People generally see the end result after the command line passes through both of these layers, which is part of what makes this so confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants