gherkin: Pass the content type of a docstring down into its pickle string form… #292
Conversation
… for use in formatters.
|
cucumber/cucumber-jvm#1265 <- this is the PR for cucumber-jvm |
Rebase the docstrings pickle acceptance test data.
|
Discovered the make build tests - hadn't noticed before. Updated the acceptance tests data. Switched to use snake_case for the json object key as I assumed that was the convention. |
| @@ -152,7 +152,8 @@ private void compileScenarioOutline(List<Pickle> pickles, List<PickleStep> backg | |||
| result.add( | |||
| new PickleString( | |||
| pickleLocation(ds.getLocation()), | |||
| interpolate(ds.getContent(), variableCells, valueCells) | |||
| interpolate(ds.getContent(), variableCells, valueCells), | |||
| ds.getContentType() | |||
There was a problem hiding this comment.
Shouldn't this also be interpolated?
|
I hadn’t quite understood its significance. I see it’s for scenario outlines with examples. Can do so. What I’m not sure of if is the docstring content type can be used within a step. Eg can you use a PickleString as an argument in a step and get the whole thing rather than just its contents? It seems that it could be useful if not. Also may be more useful with the Transformers rework - could we discriminate transformers by content type.
Bit of a cross repo conversation ...
… On 22 Oct 2017, at 13:45, M.P. Korstanje ***@***.***> wrote:
@mpkorstanje commented on this pull request.
In gherkin/java/src/main/java/gherkin/pickles/Compiler.java:
> @@ -152,7 +152,8 @@ private void compileScenarioOutline(List<Pickle> pickles, List<PickleStep> backg
result.add(
new PickleString(
pickleLocation(ds.getLocation()),
- interpolate(ds.getContent(), variableCells, valueCells)
+ interpolate(ds.getContent(), variableCells, valueCells),
+ ds.getContentType()
Shouldn't this also be interpolated?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
I think it might be possible to do this:
Scenario: The parser accepts formatted text
Given some formatted text
"""<format>
<header>Hello world!<footer>
"""
Example:
| format | header | footer|
| html | <p> | </p> |
| xml | <type:paragraph> | </type:paragraph> |
And while the content type is not passed onto the step definition, it might still be useful to the formatter to properly highlight xml or html.
Not a major use case I imagine but it also doesn't seem to hurt.
|
We're on slack btw. https://cucumberbdd-slack-invite.herokuapp.com/ the #commiters-jvm channel. |
brasmusson
changed the title
| @@ -1 +1 @@ | |||
| {"pickle":{"language":"en","locations":[{"column":3,"line":3}],"name":"minimalistic","steps":[{"arguments":[{"content":"first line (no indent)\n second line (indented with two spaces)\n\nthird line was empty","location":{"column":7,"line":5}}],"locations":[{"column":11,"line":4}],"text":"a simple DocString"},{"arguments":[{"content":"<foo>\n <bar />\n</foo>","location":{"column":7,"line":12}}],"locations":[{"column":11,"line":11}],"text":"a DocString with content type"},{"arguments":[{"content":"wrongly indented line","location":{"column":7,"line":18}}],"locations":[{"column":9,"line":17}],"text":"a DocString with wrong indentation"},{"arguments":[{"content":"first line\nsecond line","location":{"column":7,"line":22}}],"locations":[{"column":9,"line":21}],"text":"a DocString with alternative separator"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":27}}],"locations":[{"column":9,"line":26}],"text":"a DocString with normal separator inside"},{"arguments":[{"content":"first line\n```\nthird line","location":{"column":7,"line":33}}],"locations":[{"column":9,"line":32}],"text":"a DocString with alternative separator inside"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":39}}],"locations":[{"column":9,"line":38}],"text":"a DocString with escaped separator inside"}],"tags":[]},"type":"pickle","uri":"testdata/good/docstrings.feature"} | |||
| {"pickle":{"language":"en","locations":[{"column":3,"line":3}],"name":"minimalistic","steps":[{"arguments":[{"content":"first line (no indent)\n second line (indented with two spaces)\n\nthird line was empty","location":{"column":7,"line":5}}],"locations":[{"column":11,"line":4}],"text":"a simple DocString"},{"arguments":[{"content":"<foo>\n <bar />\n</foo>","content_type":"xml","location":{"column":7,"line":12}}],"locations":[{"column":11,"line":11}],"text":"a DocString with content type"},{"arguments":[{"content":"wrongly indented line","location":{"column":7,"line":18}}],"locations":[{"column":9,"line":17}],"text":"a DocString with wrong indentation"},{"arguments":[{"content":"first line\nsecond line","location":{"column":7,"line":22}}],"locations":[{"column":9,"line":21}],"text":"a DocString with alternative separator"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":27}}],"locations":[{"column":9,"line":26}],"text":"a DocString with normal separator inside"},{"arguments":[{"content":"first line\n```\nthird line","location":{"column":7,"line":33}}],"locations":[{"column":9,"line":32}],"text":"a DocString with alternative separator inside"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":39}}],"locations":[{"column":9,"line":38}],"text":"a DocString with escaped separator inside"}],"tags":[]},"type":"pickle","uri":"testdata/good/docstrings.feature"} | |||
There was a problem hiding this comment.
This file should not be edited directly, it is gherkin/testdata/good/docstrings.feature.pickles.ndjson which is the source file that is pushed down to each language version of the gherkin library (using rsync_files). Since we use the same testdata for acceptance tests for all language version for the gherkin library, this pull-request cannot be merged until all 8 language versions have been updated.
|
Do you expect each contributor to fix all 8 languages, including those they’ve not used?
… On 22 Oct 2017, at 15:38, Björn Rasmusson ***@***.***> wrote:
@brasmusson commented on this pull request.
In gherkin/java/testdata/good/docstrings.feature.pickles.ndjson:
> @@ -1 +1 @@
-{"pickle":{"language":"en","locations":[{"column":3,"line":3}],"name":"minimalistic","steps":[{"arguments":[{"content":"first line (no indent)\n second line (indented with two spaces)\n\nthird line was empty","location":{"column":7,"line":5}}],"locations":[{"column":11,"line":4}],"text":"a simple DocString"},{"arguments":[{"content":"<foo>\n <bar />\n</foo>","location":{"column":7,"line":12}}],"locations":[{"column":11,"line":11}],"text":"a DocString with content type"},{"arguments":[{"content":"wrongly indented line","location":{"column":7,"line":18}}],"locations":[{"column":9,"line":17}],"text":"a DocString with wrong indentation"},{"arguments":[{"content":"first line\nsecond line","location":{"column":7,"line":22}}],"locations":[{"column":9,"line":21}],"text":"a DocString with alternative separator"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":27}}],"locations":[{"column":9,"line":26}],"text":"a DocString with normal separator inside"},{"arguments":[{"content":"first line\n```\nthird line","location":{"column":7,"line":33}}],"locations":[{"column":9,"line":32}],"text":"a DocString with alternative separator inside"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":39}}],"locations":[{"column":9,"line":38}],"text":"a DocString with escaped separator inside"}],"tags":[]},"type":"pickle","uri":"testdata/good/docstrings.feature"}
+{"pickle":{"language":"en","locations":[{"column":3,"line":3}],"name":"minimalistic","steps":[{"arguments":[{"content":"first line (no indent)\n second line (indented with two spaces)\n\nthird line was empty","location":{"column":7,"line":5}}],"locations":[{"column":11,"line":4}],"text":"a simple DocString"},{"arguments":[{"content":"<foo>\n <bar />\n</foo>","content_type":"xml","location":{"column":7,"line":12}}],"locations":[{"column":11,"line":11}],"text":"a DocString with content type"},{"arguments":[{"content":"wrongly indented line","location":{"column":7,"line":18}}],"locations":[{"column":9,"line":17}],"text":"a DocString with wrong indentation"},{"arguments":[{"content":"first line\nsecond line","location":{"column":7,"line":22}}],"locations":[{"column":9,"line":21}],"text":"a DocString with alternative separator"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":27}}],"locations":[{"column":9,"line":26}],"text":"a DocString with normal separator inside"},{"arguments":[{"content":"first line\n```\nthird line","location":{"column":7,"line":33}}],"locations":[{"column":9,"line":32}],"text":"a DocString with alternative separator inside"},{"arguments":[{"content":"first line\n\"\"\"\nthird line","location":{"column":7,"line":39}}],"locations":[{"column":9,"line":38}],"text":"a DocString with escaped separator inside"}],"tags":[]},"type":"pickle","uri":"testdata/good/docstrings.feature"}
This file should not be edited directly, it is gherkin/testdata/good/docstrings.feature.pickles.ndjson which is the source file that is pushed down to each language version of the gherkin library (using rsync_files). Since we use the same testdata for acceptance tests for all language version for the gherkin library, this pull-request cannot be merged until all 8 language versions have been updated.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@rjwittams Many people can contribute to a pull-request, so "each contributor" does not need to fix all languages. However the acceptance test data should not diverge between language versions. |
The Pickles are primarily for executing the test cases from each feature file, so if the content type is not needed for that, maybe the content type should not be added to the Pickles data. A formatter can still access the content type, since the formatter has access to the complete feature file content. |
|
Ie each reporter should reparse the feature file? Does that seem right?
What is the purpose of the content type? Given that it is not in the Pickle, it can not be used in the test execution. However, that is an arbitrary choice - argument transformers could make use of it. In fact I was planning to do this.
If there is no point in continuing let me know. It is however a clear regression in Cucumber JVM vs 1.2.5 .
… On 22 Oct 2017, at 16:54, Björn Rasmusson ***@***.***> wrote:
And while the content type is not passed onto the step definition, ...
The Pickles are primarily for executing the test cases from each feature file, so if the content type is not needed for that, maybe the content type should not be added to the Pickles data.
A formatter can still access the content type, since the formatter has access to the complete feature file content.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
This is a real argument for including the content type in the Pickle. That it is easier for the formatters is not. For instance the step keyword is not in the Pickle, even though practically every formatter needs to use it. |
|
Also, as mentioned above, it may be useful (and consistent) for it to be transformed by interpolation. |
Added a test for this. Updated other language implementations. The Obj-C implementation does not appear to support pickles so I didn't change it. There is a generated parser change in the javascript. Not sure why - if it was accidentally left out before, or is environmental ( different versions of something?)
| @@ -48,9 +48,6 @@ module.exports = function Parser(builder) { | |||
| if(typeof tokenScanner == 'string') { | |||
| tokenScanner = new TokenScanner(tokenScanner); | |||
| } | |||
| if(typeof tokenMatcher == 'string') { | |||
There was a problem hiding this comment.
Not sure why this change was made in the generated parser - I assume it was missed out. I assume it is benign.
|
The build seemed to fail due to yarn timing out. Can it be restarted? |
|
@rjwittams Restarted the build. It fails with the following message: No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself. |
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs. |
stale
bot
added
the
⌛ stale
stale
bot
removed
the
⌛ stale
|
Hi @rjwittams, Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾 In return for this generous offer we hope you will:
On behalf of the Cucumber core team, |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
… for use in formatters.
Summary
Pass the content type of a docstring down into its pickle string form.
This allows formatters to use it.
This used to work in Cucumber 1.2.5.
A PR for cucumber-jvm accompanies this.
Details
Motivation and Context
cucumber/cucumber-jvm#1263
As suggested here by mpkorstanje , I am doing a PR against the mono repo.
How Has This Been Tested?
Added in a unit test - not sure if it should be moved.
Also not sure that Gson-ing pickles is expected - given camelCase vs snake_case.
Ran against my updated cucumber-jvm. Added a unit test there.
Verified that my json report processor which works with 1.2.5 picks up the docstrings.
Ran all tests in cucumber-jvm. Examples: Pax-Exam : Calculator Test is broken before and after the change due to a missing deployment I think.
Screenshots (if appropriate):
Types of changes
Adds a field to PickleString. Not sure if that gets serialised within this project.
Maintains the old constructor signature to avoid compile breaks. Could be deprecated.
Checklist:
Not sure which if any docs should be updated - I think 1.2.5 preserved docs will now be correct.