Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[java] Improve comments prettyPrinting #20

Merged
merged 3 commits into from
May 25, 2023
Merged

[java] Improve comments prettyPrinting #20

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bd52bc0
Improve comment management
blacelle Apr 2, 2023
8c7a946
Add changelog entry
blacelle Apr 2, 2023
e8813c0
Merge branch 'main' into improveComments
blacelle May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/)
and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]
### Changed
- [Java] Improve prettyPrint of comments

## [8.0.2] - 2022-11-21
### Added
Expand Down
56 changes: 39 additions & 17 deletions java/src/main/java/io/cucumber/gherkin/utils/pretty/PrettyHandlers.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,8 @@ public Result handleScenario(Scenario scenario, Result result) {

@Override
public Result handleStep(Step step, Result result) {
appendComments(step.getLocation(), result, comments, scenarioLevel + 1);
appendComments(step.getLocation(), result, comments, scenarioLevel + 1, true);
appendComments(step.getLocation(), result, comments, scenarioLevel + 1, false);
return result.append(stepPrefix(scenarioLevel + 1, syntax))
.append(step.getKeyword())
.append(step.getText())
Expand Down Expand Up @@ -140,11 +141,13 @@ private Result appendScenario(
Scenario stepContainer,
Syntax syntax,
int level) {
appendComments(stepContainer.getLocation(), result, comments, level, true);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level, false);

List<Tag> tags = stepContainer.getTags() != null ? stepContainer.getTags() : Collections.emptyList();
int stepCount = stepContainer.getSteps() != null ? stepContainer.getSteps().size() : 0;
String description = prettyDescription(stepContainer.getDescription(), syntax);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level);
appendTags(result, tags, syntax, level, comments);
return result
.append(keywordPrefix(level, syntax))
Expand All @@ -160,8 +163,11 @@ private Result appendFeature(
Feature stepContainer,
Syntax syntax,
int level) {
appendComments(stepContainer.getLocation(), result, comments, level, true);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level, false);

List<Tag> tags = stepContainer.getTags() != null ? stepContainer.getTags() : Collections.emptyList();
appendComments(stepContainer.getLocation(), result, comments, level);
appendTags(result, tags, syntax, level, comments);
return result.append(level == 0 ? "" : "\n")
.append(keywordPrefix(level, syntax))
Expand All @@ -178,10 +184,12 @@ private Result appendRule(
Syntax syntax,
int level
) {
appendComments(stepContainer.getLocation(), result, comments, level, true);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level, false);

List<Tag> tags = stepContainer.getTags() != null ? stepContainer.getTags() : Collections.emptyList();
String description = prettyDescription(stepContainer.getDescription(), syntax);
appendComments(stepContainer.getLocation(), result, comments, level);
result.append(level == 0 ? "" : "\n");
appendTags(result, tags, syntax, level, comments);
return result
.append(keywordPrefix(level, syntax))
Expand All @@ -198,10 +206,12 @@ private Result appendExamples(
Syntax syntax,
int level
) {
appendComments(stepContainer.getLocation(), result, comments, level, true);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level, false);

List<Tag> tags = stepContainer.getTags() != null ? stepContainer.getTags() : Collections.emptyList();
String description = prettyDescription(stepContainer.getDescription(), syntax);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level);
appendTags(result, tags, syntax, level, comments);
return result
.append(keywordPrefix(level, syntax))
Expand All @@ -218,10 +228,13 @@ private Result appendBackground(
Syntax syntax,
int level
) {
appendComments(stepContainer.getLocation(), result, comments, level, true);
result.append(level == 0 ? "" : "\n");
appendComments(stepContainer.getLocation(), result, comments, level, false);

int stepCount = stepContainer.getSteps() != null ? stepContainer.getSteps().size() : 0;
String description = prettyDescription(stepContainer.getDescription(), syntax);
appendComments(stepContainer.getLocation(), result, comments, level);
return result.append(level == 0 ? "" : "\n")
return result
.append(keywordPrefix(level, syntax))
.append(stepContainer.getKeyword())
.append(": ").append(stepContainer.getName())
Expand All @@ -245,7 +258,7 @@ private Result appendTags(Result result, List<Tag> tags, Syntax syntax, int leve
}
String prefix = syntax == Syntax.gherkin ? repeatString(level, " ") : "";
String tagQuote = syntax == Syntax.gherkin ? "" : "`";
appendComments(tags.get(0).getLocation(), result, comments, level);
appendComments(tags.get(0).getLocation(), result, comments, level, false);
return result
.append(prefix)
.append(tags.stream()
Expand Down Expand Up @@ -334,14 +347,19 @@ private Result appendTableRows(
return result;
}

private List<Comment> popComments(Location currentLocation, List<Comment> comments) {
private List<Comment> popComments(Location currentLocation, List<Comment> comments, int level, boolean previousBlock) {
List<Comment> res = new ArrayList<>();
Iterator<Comment> iter = comments.iterator();
while (iter.hasNext()) {
Comment comment = iter.next();
if (currentLocation.getLine() > comment.getLocation().getLine()) {
res.add(comment);
iter.remove();
if (previousBlock && comment.getText().startsWith(repeatString(level, " "))) {
// We skip this comment as we are appending previous block comments, and this comment has enough indents
// to be attached to the current block
} else {
res.add(comment);
iter.remove();
}
}
}
return res;
Expand All @@ -356,7 +374,7 @@ private Result appendTableRow(
List<Comment> comments) {
int actualLevel = syntax == Syntax.markdown ? 1 : level;

appendComments(row.getLocation(), result, comments, actualLevel);
appendComments(row.getLocation(), result, comments, actualLevel, false);

result.append(repeatString(actualLevel, " "))
.append("| ");
Expand All @@ -374,10 +392,14 @@ private Result appendTableRow(
return result.append(" |\n");
}

private void appendComments(Location location, Result result, List<Comment> comments, int actualLevel) {
for (Comment nextComment : popComments(location, comments)) {
private Result appendComments(Location location, Result result, List<Comment> comments, int level, boolean previousBlock) {
int actualLevel = previousBlock ? level - 1 : level;

for (Comment nextComment : popComments(location, comments, level, previousBlock)) {
appendComment(actualLevel, result, nextComment);
}

return result;
}

private void appendComment(int actualLevel, Result result, Comment nextComment) {
Expand Down
5 changes: 5 additions & 0 deletions java/src/main/java/io/cucumber/gherkin/utils/pretty/Result.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,4 +25,9 @@ public Long getDeepestLine() {
public String toString() {
return builder.toString();
}

public boolean endsWithEol() {
int length = builder.length();
return length >= 1 && builder.charAt(length - 1) == '\n';
}
}
114 changes: 114 additions & 0 deletions java/src/test/java/io/cucumber/gherkin/utils/pretty/PrettyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,120 @@ public void commentAtStartAndEndOfFile() {
prettyPrint(gherkinDocument, Syntax.gherkin));
}

@Test
void commentBeforeExamples() {
GherkinDocument gherkinDocument = parse("Feature: Comment before an Examples\n" +
"\n" +
" Scenario Outline: with examples\n" +
" Given the <value> minimalism\n" +
" # then something happens\n" +
"\n" +
" Examples:\n" +
" | value |\n" +
" | 1 |\n" +
" | 2 |\n");
assertEquals("Feature: Comment before an Examples\n" +
"\n" +
" Scenario Outline: with examples\n" +
" Given the <value> minimalism\n" +
"\n" +
" # then something happens\n" +
" Examples:\n" +
" | value |\n" +
" | 1 |\n" +
" | 2 |\n",
prettyPrint(gherkinDocument, Syntax.gherkin));
}

// This demonstrate some limitations like:
// - Comments between a `@tag` and a `Scenario:` are pushed before the @tag
// - Dangling comments (e.g. with intermediate EOL) are compacted next to some block
// - Contiguous `###[...]` are turned into `# ##[...]`
// - Comments after an moreIndented block are moved as indent of the following lessIndented block
@Test
void commentsBeforeAndAfterKeywords(){
GherkinDocument gherkinDocument = parse("# before Feature\n" +
"Feature: Comments everywhere\n" +
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it would be useful to harmonize our test cases between languages. How this might look:

  1. new directory testdata/pretty/before and testdata/pretty/after
  2. @blacelle move our examples into before/foo.feature and after/foo.feature
  3. we read these files from disk in our JS/Java test code, and both suites execute all examples

The benefit is, we ensure that both languages have identical treatment of comments when prettying. I am not sure how important this behavior is.

If interested, I can get this started on #16.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benefit is, we ensure that both languages have identical treatment of comments when prettying. I am not sure how important this behavior is.

This would definitely be a good thing.

I would be happy to follow this structure, preferable in a separate PR. But I do not get how to contribute to this repository as this PR seems stuck :S.

"# after Feature\n" +
" # after Feature indent\n" +
"\n" +
" # before Background indent\n" +
" # before Background\n" +
" Background: foo\n" +
" # after Background\n" +
"\n" +
" # before Scenario\n" +
" Scenario: foo\n" +
" # after Scenario\n" +
" # before Given\n" +
" Given nothing\n" +
" # after Given\n" +
"\n" +
" #####\n" +
" # And spaced out\n" +
" ####\n" +
"\n" +
" # before Tag\n" +
" @foo\n" +
" # after Tag\n" +
" Scenario: Foo\n" +
" Given another\n" +
"\n" +
" # before Rule\n" +
" Rule:\n" +
" # after Rule\n" +
" # after Rule indent\n" +
"\n" +
" # background comment\n" +
" Background: foo\n" +
"\n" +
" # middling comment\n" +
"\n" +
" # another middling comment\n" +
"\n" +
" # scenario comment\n" +
" Scenario: foo\n");
assertEquals( "# before Feature\n" +
"Feature: Comments everywhere\n" +
"# after Feature\n" +
"\n" +
" # after Feature indent\n" +
" # before Background indent\n" +
" # before Background\n" +
" Background: foo\n" +
"\n" +
" # after Background\n" +
" # before Scenario\n" +
" Scenario: foo\n" +
" # after Scenario\n" +
" # before Given\n" +
" Given nothing\n" +
"\n" +
" # after Given\n" +
" # ####\n" +
" # And spaced out\n" +
" # ###\n" +
" # before Tag\n" +
" # after Tag\n" +
" @foo\n" +
" Scenario: Foo\n" +
" Given another\n" +
"\n" +
" # before Rule\n" +
" Rule: \n" +
" # after Rule\n" +
"\n" +
" # after Rule indent\n" +
" # background comment\n" +
" Background: foo\n" +
"\n" +
" # middling comment\n" +
" # another middling comment\n" +
" # scenario comment\n" +
" Scenario: foo\n",
prettyPrint(gherkinDocument, Syntax.gherkin));
}

private GherkinDocument parse(String gherkin) {
GherkinParser parser = GherkinParser
.builder()
Expand Down