Multi command #4456
Merged
Multi command #4456
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
One of the commits says "Do not require leading space for multi command." This should be do not require leading semicolon? |
|
Fixes #3051 |
|
Yes, that is correct. It should say semicolon. I am going to have to revise this PR so I'll update the commit message when I do. |
eed3si9n
reviewed
Nov 19, 2018
|
|
||
| > checkStringValue bar | ||
|
|
||
| > ~;setStringValue foo;setStringValue bar; checkStringValue bar |
There was a problem hiding this comment.
In this example, is the leading semicolon required, or optional?
There was a problem hiding this comment.
optional. I'll duplicate that test case without the leading ;.
|
More awesome work, @eatkins! |
eed3si9n
reviewed
Nov 19, 2018
Prior to this commit, there was no unit testing of the parser for multiple commands. I wanted to make some improvements to the parser, so I reworked the implementation to be testable. This change also allows the multiParserImpl method to be shared with Watched.watch, which I will also update in a subsequent commit. There also were no explicit scripted tests for multiple commands, so I added one that I will augment in later commits.
I discovered that when I ran multi-commands with '~' that if there was a space between the ';' and the command, then the parsing of the command would fail and the watch would abort. To fix this, I refactor Watched.watch to use the multi command parser and, if that parser fails, we fallback on a single command.
Presently the multi command parser doesn't work correctly if one of the commands includes a string literal. For example, suppose that there is an input task defined name "bash" that shells out and runs the input. Then the following does not work with the current multi command parser: ; bash "rm target/classes/Foo.class; touch src/main/scala/Foo.scala"; comple Note that this is a real use case that has caused me issues in the past. The problem is that the semicolon inside of the quote gets interpreted as a command separator token. To fix this, I rework the parser so that it consumes string literals and doesn't modify them. By using StringEscapable, I allow the string to contain quotation marks itself. I couldn't write a scripted test for this because in a command like `; foo "bar"; baz`, the quotes around bar seem to get stripped. This could be fixed by adding an alternative to StringEscapable that matches an escaped string, but that is more work than I'm willing to do right now.
It has long been a frustration of mine that it is necessary to prepend multiple commands with a ';'. In this commit, I relax that restriction. I had to reorder the command definitions so that multi comes before act. This was because if the multi command did not have a leading semicolon, then it would be handled by the action parser before the multi command parser had a shot at it. Sadness ensued.
eatkins
commented
Nov 20, 2018
| (part map (_.trim)).+ map (_.toList) | ||
| def commandParser = state.map(s => (s.combinedParser & cmdPart) | cmdPart).getOrElse(cmdPart) | ||
| val part = semi.flatMap(_ => matched(commandParser) <~ token(OptSpace)).map(_.trim) | ||
| (cmdPart.? ~ part.+).map { |
There was a problem hiding this comment.
The cmdPart.? ~ part.+ is what ensures that a ; separator somewhere in the command is necessary for the parser to succeed. This is because part must start with a semicolon.
eed3si9n
approved these changes
Nov 20, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR enhances the parser for multiple commands/tasks and shares the improved parser with the command parser used by Watched.watch so that there should be parity between which commands are valid on the command line and which commands are valid in a continuous build.
The two main features this PR adds is
it would not work correctly because the
;in the bash command string would be interpreted as a command separator. Now the string will be interpreted as a literal value and the;in the bash command string is not considered a token.2. drop the requirement that multiple commands need to be prefixed with a leading ';'. Now the command above could just be
Also, the
~will now use the multi command parser rather than the naive string split on;that it was doing before. This should make it so that any multi command that is valid on the console can be prefixed with~and things should work mostly as expected.I also verified manually that tab completions still work for
;prefixed tasks/commands.Fixes #3051