Add console string quoting. #705
Conversation
|
Is there some reason this pull request has not been acted on? Is the ArduinoBuild / Build ESP32 examples build failure check holding it up? |
that can be ignored as it is trying to use the latest arduino-esp32 that isn't supported on master (and won't be anytime soon). |
|
So, what is holding up the pull request? I would like to be synced and up-to-date, but I need this update for my PocketBeagle command station, so the version of OpenMRN on my BeagleBone build box is being held back waiting for this PR... |
|
Is there something I need to do to get this pull request approved? |
src/console/Console.cxx
Outdated
| case '"': | ||
| /** @todo quoted arguments not yet supported */ | ||
| /// Quote handling added (Robert Heller <[email protected]>) |
There was a problem hiding this comment.
This is not necessary, the git history keeps track of who added what code and why.
If you are looking to permanently record your name for the licensing, add it to line 30.
| else if (quote == '\0') | ||
| { /* Start of quoted */ | ||
| quote = line[i]; // Save the quote mark | ||
| if ((i + 1) < pos) |
There was a problem hiding this comment.
instead of this if, which duplicates code from elsewhere, it would be enough to have these two statements:
last = quote;
line[i] = '\0';
There was a problem hiding this comment.
I am not sure I understand. Might it be you mean:
last = '\0';
?
And what about a check for a loose quote at EOL?
src/console/Console.cxxtest
Outdated
| @@ -22,7 +22,7 @@ TEST(ConsoleTest, testHelp) | |||
| " has an effect on socket based logins sessions\n" | |||
| "> ", 154)); | |||
|
|
|||
| EXPECT_EQ(::write(s, "?\n", 3), 3); | |||
| EXPECT_EQ(::write(s, "?\n", 5), 5); | |||
There was a problem hiding this comment.
This change does not seem right. There are only three characters in that string (question mark, newline, zero). We should not write 5 bytes here.
src/console/Console.cxxtest
Outdated
| if (argc == 0) { | ||
| fprintf(fp, "the echo command\n"); | ||
| } else if (argc == 2) { | ||
| fprintf(fp, "%s\n", argv[2]); |
There was a problem hiding this comment.
argv[1]
(because argv[argc] should always be NULL)
| EXPECT_EQ(::write(s, "echo '\"Oh no, no, no\", said the man'\n", 37), 37); | ||
| usleep(1000); | ||
| EXPECT_EQ(::read(s, buf, 1024), 32); | ||
| EXPECT_TRUE(!strncmp(buf, "\"Oh no, no, no\", said the man\n> ", 32)); |
There was a problem hiding this comment.
consider adding another test case with mismatched quote to exercise the error path as well. This will be helpful to keep test coverage high.
…ror tests in Console.cxxtest.
This is a new attempt to get this into the root fork. I need this patch for my command station code. This is a feature enhancement to allow consoles to allow quoting command line parameters, specificly to allow command line parameters with spaces and other word break characters.