Gherkin-Utils - Java - Address issue #1662 - add a Java pretty print gherkin module #1725

nddipiazza · 2021-09-03T03:46:23Z

Summary

Ported the TypeScript Gherkin pretty print utility to Java.

Details

Make a java port of the javascript pretty printing library available in gherkin-utils.

Motivation and Context

A huge amount of the Gherkin users out there are java users through cucumber-jvm project. We need a pretty printer for Gherkin.

How Has This Been Tested?

Took most of the unit tests from the typescript project.

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

The change has been ported to Java.
I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have updated the CHANGELOG accordingly.

…s/1662

…1662

do my best at trying to add the make and gherkin-ish stuff

aslakhellesoy · 2021-09-05T21:41:24Z

Wow thanks so much for contributing this! I haven't had a chance to look at it yet, but will do my best to do it this week.

aslakhellesoy

Overall great code! I've left some comments, mostly about deleting unnecessary files that aren't needed.

Also please add an entry to the CI build configuration in .circleci/config.yml to make sure this gets built.

aslakhellesoy · 2021-09-06T14:39:54Z

gherkin-utils/java/.rsync

+../../.templates/github/ .github/
+../../.templates/java/ .
+../testdata/ testdata/
+../gherkin.berp gherkin.berp


Please remove this line - it's for the gherkin library only

aslakhellesoy · 2021-09-06T14:40:31Z

gherkin-utils/java/.subrepo

@@ -0,0 +1 @@
+cucumber/gherkin-java


Please remove this file

aslakhellesoy · 2021-09-06T14:40:38Z

gherkin-utils/java/.rsync

+../../.templates/java/ .
+../testdata/ testdata/
+../gherkin.berp gherkin.berp
+../gherkin-languages.json src/main/resources/io/cucumber/gherkin/gherkin-languages.json


Remove this line too.

aslakhellesoy · 2021-09-06T14:41:23Z

gherkin-utils/java/LICENSE

@@ -0,0 +1,21 @@
+The MIT License (MIT)
+
+Copyright (c) Cucumber Ltd, Gaspar Nagy, Björn Rasmusson, Peter Sergeant


Only Cucumber Ltd please, and optionally your own name.

aslakhellesoy · 2021-09-06T14:41:49Z

gherkin-utils/java/Makefile

@@ -0,0 +1,53 @@
+include default.mk
+


Please remove everything below this line.

aslakhellesoy · 2021-09-06T14:42:59Z

gherkin-utils/java/gherkin-java.razor

@@ -0,0 +1,320 @@
+//      This code was generated by Berp (http://https://github.com/gasparnagy/berp/).


Please remove this file

aslakhellesoy · 2021-09-06T14:43:15Z

gherkin-utils/java/gherkin.berp

@@ -0,0 +1,38 @@
+[


Please remove this file

aslakhellesoy · 2021-09-06T14:44:19Z

gherkin-utils/java/pom.xml

+  <modelVersion>4.0.0</modelVersion>
+  <groupId>io.cucumber</groupId>
+  <artifactId>gherkin-utils</artifactId>
+  <version>21.0.1-SNAPSHOT</version>


Please align the version number with the existing gherkin-utils/javascript version (and add 1 for the patch version plus a -SNAPSHOT suffix).

aslakhellesoy · 2021-09-06T14:46:10Z

gherkin-utils/java/pom.xml

+
+  <dependencies>
+    <dependency>
+      <groupId>io.cucumber</groupId>


This should be test scope.

Please add a runtime dependency on messages

i could not make this change, it caused project not to compile due to missing dependencies during compile time.

aslakhellesoy · 2021-09-06T14:47:52Z

gherkin-utils/java/src/main/java/io/cucumber/gherkin/util/PrettyPrintGherkinDocument.java

+                (description != null && !"".equals(description.trim()) && stepCount > 0 ? "\n" : "");
+    }
+
+    private static String prettyKeywordContainer(


There is a lot of duplication in this method and the next three. Can you extract a private method that they all use?

jamietanna · 2021-09-07T05:52:45Z

As mentioned in #1729 I've literally just finished working on this as an external project 😭 (sorry for not raising an issue to share that I was!)

So I'd be able to help it you'd like with implementation / we can see about pulling my library in here?

jamietanna · 2021-09-07T06:02:24Z

gherkin-utils/java/src/test/java/io/cucumber/gherkin/util/PrettyPrintGherkinDocumentTest.java

+    }
+
+    @Test
+    public void scenarioOutlineWithExamples() {


To make these test classes more readable, we could split the cases into files before/after, then assert on that, which means we don't have to escape / deal with long strings in this test class

jamietanna · 2021-09-07T06:10:42Z

gherkin-utils/java/pom.xml

+
+    <dependency>
+      <groupId>junit</groupId>
+      <artifactId>junit</artifactId>


❓ are we still as a project using JUnit4? Or are we able to update this to JUnit5?

We can move to junit5, we use that in a few other places

aslakhellesoy · 2021-09-07T06:16:36Z

As mentioned in #1729 I've literally just finished working on this as an external project 😭 (sorry for not raising an issue to share that I was!)

So I'd be able to help it you'd like with implementation / we can see about pulling my library in here?

That would be great @jamietanna! Just keep in mind that we aim to keep the TypeScript/Java implementations consistent.

jamietanna

Can we please make sure this is testing against everything in gherkin/testadata - looks like escaped_pipes.feature returns a slightly different result that I'd expect (\\o vs \o) and there are likely a few other edge cases

aslakhellesoy · 2021-09-07T14:41:49Z

Can we please make sure this is testing against everything in gherkin/testadata - looks like escaped_pipes.feature returns a slightly different result that I'd expect (\o vs \o) and there are likely a few other edge cases

The files in gherkin/testadata are (intentionally) not formatted according to the pretty implementation. We can read them all in and format them (the tests in the TypeScript implementation does this), but it doesn't compare the formatted document against anything - we just verify that we can parse the formatted document again.

I think the best way to ensure edge cases are covered is to write dedicated unit tests where we have control over the expected output.

jamietanna · 2021-09-07T15:06:21Z

Sorry, so what I mean is that there are cases where this implementation does not handle specific cases i.e. not correctly escaping in escaped_pipes.feature.

I'd recommend, similar to how I've done it on my own implementation, we have a set of tests via testdata (and other sources, i.e. cases we've written) that source the "before" view, and then validate that it pretty prints to a set of known outputs.

nddipiazza · 2021-09-07T15:47:14Z

OK do you have a list of the test feature files somewhere you want me to add to the unit tests? I'm happy to do so.
I probably won't finish these updates requested for a few days, doing it in my spare time as my day job is eating up my day on other fires.

jamietanna · 2021-09-07T15:50:46Z

Not to worry @nddipiazza!

You can use https://gitlab.com/jamietanna/gherkin-formatter/-/tree/main/src/test/resources/input as the inputs, and https://gitlab.com/jamietanna/gherkin-formatter/-/tree/main/src/test/resources/output as outputs (for passing tests)

Which are following a formatting style I've found useful on my own implementation.

These were largely adapted from https://github.com/cucumber/gherkin-java/tree/master/testdata but I added quite a few additional scenarios as it was required to make sure I had everything really covered.

nddipiazza · 2021-09-07T15:51:15Z

Excellent. I'm on it.

jamietanna · 2021-09-07T15:52:29Z

Feel free to copy code / thinking from the library, too - it's MIT so we can bring it into this project as needed.

There's some additional complexities that don't appear to be in the Typescript version (like comments) so it may be there's a bit of extra work rather than just a plain port 😅

aslakhellesoy · 2021-09-07T19:38:03Z

Yes, comments are lost in the current implementation, and it would be great to keep them! I think we can do that by popping them off as we traverse the other nodes, comparing line numbers.

…o address_issue_1662

nddipiazza · 2022-05-17T13:45:43Z

Oh gosh, I'm finally getting back to this. I got mixed up with other work I was doing and thought this was merged.

I fixed the comment that was pending and will look into the failing test.

This reverts commit a9a30cb.

…o address_issue_1662

aslakhellesoy · 2022-05-17T22:48:41Z

Thanks for picking this up! I’ll let you know when I’ve had a chance to review it.

nddipiazza · 2022-05-17T22:58:19Z

@jamietanna @aslakhellesoy @aurelien-reeves ready for review.

want me to add any more test cases?

aurelien-reeves

From my point of view, it looks good.

One entry in the changelog of gherkin-utills is missing to document the addition of that new java implementation :p

I would just have one question regarding the changes in check-jar.sh (see within the file)

And also, it would be a great addition to update all the readmes of the component to:

have a global introduction to gherkin-utils in the root of that component
have dedicated instructions for the javascrit and the java implementations

However, I would agree to report that as another piece of work in order to move-on with the current PR. Please, just at least open an issue, or even better, a draft PR, to keep track of that.

For the java code, I'll let other to take a look 😅

aurelien-reeves · 2022-05-18T07:42:51Z

.templates/java/scripts/check-jar.sh

+  module_path=$(echo $module_name | sed "s/\./\\\\\//g" | sed "s/-/\\\\\//g")
+  echo "Checking contents of ${jar} to see if it matches pattern: ${module_path}"
  unshaded_classes=$(unzip -l ${jar} | grep -e "\.class" | rev | cut -d' ' -f1 | rev | grep -v "^$module_path")
  if [[ "${unshaded_classes}" != "" ]]; then
-    echo "Some classes in ${jar} are not in the io.cucumber package. Rename the classes or change the maven-shade-plugin configuration."
+    echo "Some classes in ${jar} are not in the expected package matching pattern ${module_path}. Rename the classes or change the maven-shade-plugin configuration."


Could you explain those changes here?

As I added the new module, I noticed that the message was misleading. It mentioned "io.cucumber" needs to be the package, but it's actually checking for io.cucumber + {sub-module}. After spinning my tires on that, I updated the message to make it clear.

And sub-modules with - in them need to be replaced with / just like you are already doing with . that way gherkin-utils becomes io/cucumber/gherkin/utils

aslakhellesoy · 2022-05-18T15:10:15Z

I've continued this work in #1985 - closing this.

nddipiazza added 3 commits September 1, 2021 22:50

some initial commits towards https://github.com/cucumber/common/issue…

b0501c1

…s/1662

finish the Pretty document stuff.

3e85f37

add some gitignores

7cdbe75

nddipiazza mentioned this pull request Sep 3, 2021

Create a static method to pretty print gherkin cucumber/gherkin-utils#4

Closed

nddipiazza and others added 6 commits September 2, 2021 22:51

Merge branch 'main' of github.com:cucumber/common into address_issue_…

ef4f7c5

…1662

code formatting.

9a73928

maven-itize the project

ad5887f

do my best at trying to add the make and gherkin-ish stuff

add more of the other files

c36c37d

javadoc

f9e6ca2

Merge branch 'main' into address_issue_1662

1242fc1

aslakhellesoy self-assigned this Sep 5, 2021

aslakhellesoy added ⚡ enhancement Request for new functionality language: java library: gherkin-utils labels Sep 5, 2021

Merge branch 'main' into address_issue_1662

d13d33e

aslakhellesoy requested changes Sep 6, 2021

View reviewed changes

aslakhellesoy mentioned this pull request Sep 6, 2021

Add pretty-formatters for Gherkin (for Java) #1729

Closed

jamietanna reviewed Sep 7, 2021

View reviewed changes

Merge branch 'address_issue_1662' of github.com:nddipiazza/common int…

c356fe3

…o address_issue_1662

nddipiazza added 19 commits May 17, 2022 08:48

fix ci issue

a9a30cb

add entry for gherkin-util/java in ci config.yaml

ed00d2a

try to fix config error

4f627cc

try to fix config error

30d9b53

try removing it

4bcc2bd

Revert "fix ci issue"

19fe741

This reverts commit a9a30cb.

more fixes

15f7e70

fix typo in name

f992a61

add commited files for ci

95f59ef

push untracked files

26417cd

fix gherkin patch m2

4b89272

fix package names to match project

c804fa1

push source

2e3d3b2

Merge branch 'main' into address_issue_1662

3ed72e1

fix remaining issues with make

6625780

push source

330371d

Merge branch 'address_issue_1662' of github.com:nddipiazza/common int…

b90afc0

…o address_issue_1662

push untracked files

cb68862

push untracked files

e85f043

aurelien-reeves suggested changes May 18, 2022

View reviewed changes

jamietanna self-requested a review May 18, 2022 08:18

Merge branch 'main' into address_issue_1662

c5e73bf

aslakhellesoy mentioned this pull request May 18, 2022

Nddipiazza address issue 1662 #1985

Merged

aslakhellesoy closed this May 18, 2022

jamietanna mentioned this pull request Jun 26, 2022

Add further examples for valid Cucumber files to pretty-print #2024

Closed

12 tasks

		@@ -0,0 +1,21 @@
		The MIT License (MIT)

		Copyright (c) Cucumber Ltd, Gaspar Nagy, Björn Rasmusson, Peter Sergeant

		@@ -0,0 +1,320 @@
		// This code was generated by Berp (http://https://github.com/gasparnagy/berp/).

		@@ -0,0 +1 @@
		cucumber/gherkin-java

Uh oh!

Gherkin-Utils - Java - Address issue #1662 - add a Java pretty print gherkin module #1725

Gherkin-Utils - Java - Address issue #1662 - add a Java pretty print gherkin module #1725

Uh oh!

Conversation

nddipiazza commented Sep 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

aslakhellesoy commented Sep 5, 2021

Uh oh!

aslakhellesoy left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jamietanna commented Sep 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aslakhellesoy commented Sep 7, 2021

Uh oh!

jamietanna left a comment

Choose a reason for hiding this comment

Uh oh!

aslakhellesoy commented Sep 7, 2021

Uh oh!

jamietanna commented Sep 7, 2021

Uh oh!

nddipiazza commented Sep 7, 2021

Uh oh!

jamietanna commented Sep 7, 2021

Uh oh!

nddipiazza commented Sep 7, 2021

Uh oh!

jamietanna commented Sep 7, 2021

Uh oh!

aslakhellesoy commented Sep 7, 2021

Uh oh!

nddipiazza commented May 17, 2022

Uh oh!

aslakhellesoy commented May 17, 2022

Uh oh!

nddipiazza commented May 17, 2022

Uh oh!

aurelien-reeves left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nddipiazza May 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

nddipiazza commented Sep 3, 2021 •

edited

Loading

nddipiazza May 18, 2022 •

edited

Loading