terminal
terminal copied to clipboard
Fixes command-line args parsing when one-character argument is followed by `;`
Summary of the Pull Request
Changes the way _addCommandsForArg determines if the delimiter was at the beginning of the argument so that it accounts for the fact that match includes the last character of the string before it.
PR Checklist
- [x] Closes #13277
- [x] CLA signed. If not, go over here and sign the CLA
- [ ] Tests added/passed
- [ ] Documentation updated. If checked, please file a pull request on our docs repo and link it here: #xxx
- [ ] Schema updated.
- [ ] I've discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #13436
Validation Steps Performed
wt -p "u"; nt -p "u" does not cause an error
Would it be ok add them to ParseTrickyCommandlines or ParseSimpleCommandline?
Is testing that one-character argument parsed correctly enough or are there more test cases that need to be added?
ParseTrickyCommandlines seems fine to me ; the wt -p "u"; nt -p "u" test case seems good enough of a test to me 😄
(Future improvement:) Looking at
_commandDelimiterRegex, I get the feeling that this code would be a lot more robust if we'd justfind(L';')and check if the preceding character is!= L'\\'.
Yes and no.
What if the character before the \ is \? \\; is not the same as \;!
What if the character before the
\is\?\\;is not the same as\;!
The regex is currently (^;|[^\\];) and described as
Either a ; at the start of a line, or a ; preceded by any non-\ char.
There's nothing in the regex that would handle such a case. If we handle it, it must be elsewhere or not at all...
This vaguely worries me, actually. Why are we using a regex to match a character followed by ; when what we want to know is whether there is a ;? Is it just because we need to match anything except a \?
Leonard's right, we should be using a more direct/implementation-specific string parser rather than a regex. It would give us control over exactly when we match and when we "vend". :)
Hello @DHowett!
Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.
p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.
:tada:Windows Terminal Preview v1.16.252 has been released which incorporates this pull request.:tada:
Handy links: