Add splitByElement() to Gherkin step definitions for nested collection parsing#3379
Add splitByElement() to Gherkin step definitions for nested collection parsing#3379spmallette merged 2 commits into3.8-devfrom
Conversation
…n parsing
Naive split(",") breaks on nested collection tokens like l[1,2,3] inside
s[l[1,2,3],l[4,5,6]]. Adds a bracket-depth-aware splitByElement() helper to
all Gherkin step definition files across Java and every GLV, so that commas
inside nested brackets are not treated as top-level separators.
Also adds two new Gherkin scenarios to Fold.feature that exercise nested
list-of-list results (g.inject([1,2],[3,4]).fold()), along with the
corresponding traversal bindings in each GLV's translation map.
Java implementation follows PR #3216.
(tinkerpop-mxr, tinkerpop-32y, tinkerpop-ghp, tinkerpop-2ur, tinkerpop-70k, tinkerpop-188, tinkerpop-93a)
yunlingTao
left a comment
yunlingTao
left a comment
There was a problem hiding this comment.
I think this is everything needed. Closed #3216.
|
VOTE +1 |
There was a problem hiding this comment.
There are only tests here for nested list, but this PR also changes set. Should add test for set as well as test for extra-depth in nesting.
There was a problem hiding this comment.
I didn't want to use gherkin to test all possible combinations of this. The gherkin tests are for testing Gremlin not the test infrastructure itself. I think my intention was to go back to tests that we had rewritten because we lacked this functionality and bring them back to the form we once had in the Java tests. I wasn't sure how to go about doing that though and wanted this to move forward. I wonder if our tests are reaching significant enough complexity to warrant their own unit testing to some degree. I'll think about this a bit more I guess.
|
Consider a CHANGELOG entry? Though I think it might be small enough that it doesn't matter. |
|
Overall I think this change looks good and I'll give it a VOTE +1 as a standalone inclusion. I do want to point out that even with this change, nested collections in feature tests is sketchy and only works in specific cases. For example the following case looks reasonable, but does not work. The primary issue with this example is that the In general mixed collection type nesting does not work and it's not a small change to fix it. My view on feature test infrastructure is typically that I don't want complexity without necessity. I'm happy to leave these limitations if we don't intend to write tests that require mixed-type nests. |
it's mostly an internal developer issue....didn't seem necessary.
Added documentation. I think the point here is not to have this be perfect. It's a small advancement in testing that allows another dimension of assertions. |
|
VOTE +1 |
1 similar comment
|
VOTE +1 |
5 participants
This PR completes #3216 which was for Java only but implements it across all the language variants. cc/ @yunlingTao
VOTE +1