-
Notifications
You must be signed in to change notification settings - Fork 2
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
Reworked command parser #28
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the long-term plan for the legacy parser? Is that kept around until both Cog and Greenbar are using the new parser?
{{state.linenum, state.current_token}, %{state | current_token: updated}} | ||
rescue | ||
e -> | ||
IO.puts "State: #{inspect state, pretty: true}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these meant to be IO.puts
as opposed to logger messages?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those aren't meant to be there at all :( Left over debug code. Will remove.
|
||
NAME = [a-z][a-zA-Z0-9_\-]* | ||
HIPCHAT_EMOJI = \([a-zA-Z0-9_\-\+]+\) | ||
SLACK_EMOJI = :[a-zA-Z0-9_\-\+]+: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we still wanting to support emoji in commands? While it's not a huge deal, it is rather odd seeing chat-adapter specific code down in the command parser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's a decision for a different time since removing emoji support is a subtractive change. Open to the idea, though, since I don't think it's gotten much use nor have we publicized the capability.
Summary: - Previously monolithic parsing logic split into a suite of composable parsers - Relaxed grammar more closely follows typical command shell parsing behavior - Introduces string interpolation support Details: This commit adds a new command parser which features a more relaxed parsing grammar and support for string interpolation. This is achieved by breaking out the lexing and parsing logic for major data types -- pipelines, names, args/options, variables, and interpolated strings -- into separate lexer/parser pairs which are combined to create the final command parsing pipeline. In the vast majority of cases the new parser produces identical AST when compared against the existing parser. Aside from the new features the only difference between the old and new parsers is their handling of quoted strings. The old parser parsed them correctly but dropped the strings when converting the AST to a string whereas the new parser preserves quoting. `Piper.Command.Parser` and `Piper.Command.ParserOptions` have been changed to make the old and new parsers swappable. Setting the `use_legacy_parser` field on `ParserOptions` to `true` will cause Piper to parse command input with the old parser. The command parser's test suite has been changed to cover new features and ensure the legacy parser continues to work. First, the test modules for variable binding, alias expansion, command lexing, and command parsing were split into two "forks". The legacy fork captures these test modules in their current state and runs them against the legacy parser. These modules are prefixed with `Legacy`. The new fork, which consists of the same test modules but retaining their non-legacy names, have been extended to exercise the new parser.
2191860
to
23b0507
Compare
I kept the legacy parser around as a fallback in case we discover some bad behavior post-release. Once we're satisfied the new parser is working as it should we can remove the legacy bits completely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good; ship it!
Summary:
parsers
behavior
Details:
This commit adds a new command parser which features a more relaxed
parsing grammar and support for string interpolation. This is achieved
by breaking out the lexing and parsing logic for major data types --
pipelines, names, args/options, variables, and interpolated strings --
into separate lexer/parser pairs which are combined to create the final
command parsing pipeline.
In the vast majority of cases the new parser produces identical AST
when compared against the existing parser. Aside from the new features
the only difference between the old and new parsers is their handling of
quoted strings. The old parser parsed them correctly but dropped the
strings when converting the AST to a string whereas the new parser
preserves quoting.
Piper.Command.Parser
andPiper.Command.ParserOptions
have beenchanged to make the old and new parsers swappable. Setting the
use_legacy_parser
field onParserOptions
totrue
will cause Piperto parse command input with the old parser.
The command parser's test suite has been changed to cover new features
and ensure the legacy parser continues to work. First, the test modules
for variable binding, alias expansion, command lexing, and command
parsing were split into two "forks". The legacy fork captures these test
modules in their current state and runs them against the legacy
parser. These modules are prefixed with
Legacy
. The new fork, whichconsists of the same test modules but retaining their non-legacy names,
have been extended to exercise the new parser.
Fixes operable/cog#1082