-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add subscript operator to Expression to access sub-expressions by name #60
Conversation
Hey, thanks for the PR, I definitely see the advantages of the rule based query, and good call with the optional return! Could you also add a test case, so the feature remains stable in the future? |
Sure, there you go! :) |
Codecov Report
@@ Coverage Diff @@
## master #60 +/- ##
==========================================
+ Coverage 97.67% 97.69% +0.02%
==========================================
Files 9 9
Lines 646 652 +6
==========================================
+ Hits 631 637 +6
Misses 15 15
Continue to review full report at Codecov.
|
test/source/parser.cpp
Outdated
ParserGenerator<std::string> program; | ||
program.setSeparator(program["Whitespace"] << "[\t ]"); | ||
|
||
program["Word"] << "[a-z]+"; | ||
program["Yell"] << "[A-Z]+"; | ||
program["Number"] << "[0-9]+"; | ||
program["IntSubscript"] << "Word Yell? Number" >> [](auto e) { return e[1].string(); }; | ||
program["StringSubscriptOptional"] << "Number Yell? Word" >> [](auto e) { | ||
if (auto expr = e["Yell"]) return expr->string(); | ||
return std::string(); | ||
}; | ||
program["Start"] << "IntSubscript | StringSubscriptOptional"; | ||
|
||
program.setStart(program["Start"]); | ||
|
||
REQUIRE(program.run("ab 0") == "0"); | ||
REQUIRE(program.run("ab CD 1") == "CD"); | ||
REQUIRE(program.run("2 ef") == ""); | ||
REQUIRE(program.run("2 GH ij") == "GH"); |
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.
Hey, thanks for adding the test case! I do find it to be a hard to follow, as it's doing a lot of things at once. What do you think about simplifying it to a single rule that checks the new [string]
operator?
For example, something like
ParserGenerator<bool> program;
program["Word"] << "[a-z]+";
program["Yell"] << "[A-Z]+";
program["Start"] << "Word | Yell" >> [](auto e){ return bool(e["Yell"]); }
REQUIRE(!program.run("hello"));
REQUIRE(program.run("HELLO"));
should be enough.
Yes, you're absolutely right. That's a much better test case.
I had a hard time coming up with a simple test yesterday so I basically simplified the use case I had for the subscript operator.
As soon as I have some time on my PC today I copy over your suggestion.
Feb 5, 2021 10:43:43 Lars Melchior <notifications@github.com>:
… @TheLartians commented on this pull request.
----------------------------------------
In test/source/parser.cpp[#60 (comment)]:
> + ParserGenerator<std::string> program;
+ program.setSeparator(program["Whitespace"] << "[\t ]");
+
+ program["Word"] << "[a-z]+";
+ program["Yell"] << "[A-Z]+";
+ program["Number"] << "[0-9]+";
+ program["IntSubscript"] << "Word Yell? Number" >> [](auto e) { return e[1].string(); };
+ program["StringSubscriptOptional"] << "Number Yell? Word" >> [](auto e) {
+ if (auto expr = e["Yell"]) return expr->string();
+ return std::string();
+ };
+ program["Start"] << "IntSubscript | StringSubscriptOptional";
+
+ program.setStart(program["Start"]);
+
+ REQUIRE(program.run("ab 0") == "0");
+ REQUIRE(program.run("ab CD 1") == "CD");
+ REQUIRE(program.run("2 ef") == "");
+ REQUIRE(program.run("2 GH ij") == "GH");
Hey, thanks for adding the test case! I do find it to be a hard to follow, as it's doing a lot of things at once. What do you think about simplifying it to a single rule that checks the new [string] operator?
For example, something like
ParserGenerator<bool> program;
program["Word"] << "[a-z]+";
program["Yell"] << "[A-Z]+";
program["Start"] << "Word | Yell" >> [](auto e){ return bool(e["Yell"]); }
REQUIRE(!program.run("hello"));
REQUIRE(program.run("HELLO"));
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub[#60 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/ADTEN343AV7EBEC4YRMQ3L3S5O4U3ANCNFSM4XBT36QA].
[data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADUAAAA1CAYAAADh5qNwAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAAiSURBVGiB7cEBDQAAAMKg909tDjegAAAAAAAAAAAAAIB7AywZAAGURgP6AAAAAElFTkSuQmCC###24x24:true###][Tracking image][https://github.com/notifications/beacon/ADTEN37QNKKWFOYXQNH6H5DS5O4U3A5CNFSM4XBT36QKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOELI4MCQ.gif]
|
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.
Awesome, thanks for the PR and updates!
The feature is released in v2.2.0! 🎉 |
Hi there!
It might be just me, but accessing certain sub-expressions of a rule by name seems very handy for me, especially with very complicated rules. So I added this. It seems to work quite well. Especially the std::optional makes parsing expressions with optional expressions much simpler.