Skip to content

Commit

Permalink
fix: MergeYaml block indentation not as expected (#4358)
Browse files Browse the repository at this point in the history 
* fix: `MergeYaml` block indentation not as expected

* Indication where things need to change

* calculate indentation

* add todo

* Make it work with variable indent

* Fix for sequences

* cleanup

* cleanup

* comments

* extract method

* revert

* revert

* Merge yaml test for inline comments retention (#4365)

* Added test cases to check inline comment retention.

* Updated issue number to check inline comment retention.

---------

Co-authored-by: j0m17kw <[email protected]>
Co-authored-by: Peter Streef <[email protected]>

* revert

* Revert all to main (to start again)

* Make multiline scalars work

* Cleanup

* Apply suggestions from code review

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

* Add extra test `addPropertyWitCommentAboveLastLine`

* Improve IndentsVisitor, so merged multiline scalars work too

* Small improvements

* Apply formatter

* Consistently use the same style of import for nested classes

* Reuse local variable `value` instead of repeated `incomingEntry.getValue()`

* Small improvements

---------

Co-authored-by: johnpaulms <[email protected]>
Co-authored-by: j0m17kw <[email protected]>
Co-authored-by: lingenj <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tim te Beek <[email protected]>
  • Loading branch information
@pstreef @johnpaulms
@jevanlingen @github-actions @timtebeek
6 people authored Nov 25, 2024
1 parent 804abca commit 615a767
Show file tree
Hide file tree
Showing 8 changed files with 390 additions and 153 deletions.
54 changes: 7 additions & 47 deletions rewrite-core/src/main/java/org/openrewrite/internal/StringUtils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,12 @@
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Map;
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.function.Predicate;
import java.util.regex.Pattern;

public class StringUtils {
private static final Pattern LINE_BREAK = Pattern.compile("\\R");

private StringUtils() {
}

Expand Down Expand Up @@ -158,50 +158,6 @@ private static int minCommonIndentLevel(String text) {
return minIndent;
}

public static int mostCommonIndent(SortedMap<Integer, Long> indentFrequencies) {
// the frequency with which each indent level is an integral divisor of longer indent levels
SortedMap<Integer, Integer> indentFrequencyAsDivisors = new TreeMap<>();
for (Map.Entry<Integer, Long> indentFrequency : indentFrequencies.entrySet()) {
int indent = indentFrequency.getKey();
int freq;
switch (indent) {
case 0:
freq = indentFrequency.getValue().intValue();
break;
case 1:
// gcd(1, N) == 1, so we can avoid the test for this case
freq = (int) indentFrequencies.tailMap(indent).values().stream().mapToLong(l -> l).sum();
break;
default:
freq = (int) indentFrequencies.tailMap(indent).entrySet().stream()
.filter(inF -> gcd(inF.getKey(), indent) != 0)
.mapToLong(Map.Entry::getValue)
.sum();
}

indentFrequencyAsDivisors.put(indent, freq);
}

if (indentFrequencies.getOrDefault(0, 0L) > 1) {
return 0;
}

return indentFrequencyAsDivisors.entrySet().stream()
.max((e1, e2) -> {
int valCompare = e1.getValue().compareTo(e2.getValue());
return valCompare != 0 ?
valCompare :
// take the smallest indent otherwise, unless it would be zero
e1.getKey() == 0 ? -1 : e2.getKey().compareTo(e1.getKey());
})
.map(Map.Entry::getKey)
.orElse(0);
}

static int gcd(int n1, int n2) {
return n2 == 0 ? n1 : gcd(n2, n1 % n2);
}

/**
* Check if the String is null or has only whitespaces.
* <p>
Expand Down Expand Up @@ -760,4 +716,8 @@ public static int indexOfNextNonWhitespace(int cursor, String source) {
public static String formatUriForPropertiesFile(String uri) {
return uri.replaceAll("(?<!\\\\)://", "\\\\://");
}

public static boolean hasLineBreak(@Nullable String s) {
return s != null && LINE_BREAK.matcher(s).find();
}
}
5 changes: 2 additions & 3 deletions rewrite-yaml/src/main/java/org/openrewrite/yaml/CoalescePropertiesVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
import java.util.List;

public class CoalescePropertiesVisitor<P> extends YamlIsoVisitor<P> {
private final FindIndentYamlVisitor<P> findIndent = new FindIndentYamlVisitor<>(0);
private final FindIndentYamlVisitor<P> findIndent = new FindIndentYamlVisitor<>();

public CoalescePropertiesVisitor() {
}
Expand Down Expand Up @@ -55,8 +55,7 @@ public Yaml.Mapping visitMapping(Yaml.Mapping mapping, P p) {
entries.add(entry.withKey(coalescedKey)
.withValue(subEntry.getValue()));

int indentToUse = findIndent.getMostCommonIndent() > 0 ?
findIndent.getMostCommonIndent() : 4;
int indentToUse = findIndent.getMostCommonIndent() > 0 ? findIndent.getMostCommonIndent() : 4;
doAfterVisit(new ShiftFormatLeftVisitor<>(subEntry.getValue(), indentToUse));

changed = true;
Expand Down
60 changes: 30 additions & 30 deletions rewrite-yaml/src/main/java/org/openrewrite/yaml/MergeYamlVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@
import org.openrewrite.internal.ListUtils;
import org.openrewrite.style.GeneralFormatStyle;
import org.openrewrite.yaml.tree.Yaml;
import org.openrewrite.yaml.tree.Yaml.Document;
import org.openrewrite.yaml.tree.Yaml.Mapping;
import org.openrewrite.yaml.tree.Yaml.Mapping.Entry;
import org.openrewrite.yaml.tree.Yaml.Scalar;

import java.util.ArrayList;
import java.util.Arrays;
Expand All @@ -34,21 +30,22 @@
import java.util.regex.Pattern;
import java.util.stream.Collectors;

import static java.lang.System.lineSeparator;
import static org.openrewrite.Cursor.ROOT_VALUE;
import static org.openrewrite.Tree.randomId;
import static org.openrewrite.internal.ListUtils.*;
import static org.openrewrite.internal.StringUtils.hasLineBreak;
import static org.openrewrite.yaml.MergeYaml.REMOVE_PREFIX;

/**
* Visitor class to merge two yaml files.
*
* @param <P> An input object that is passed to every visit method.
* @implNote Loops recursively through the documents, for every part a new MergeYamlVisitor instance will be created.
* As inline comments are put on the prefix of the next element (which can be an item very much higher in the tree),
* the following solutions are chosen to merge the comments as well:
* <ul>
* <li>when an element has new items, the comment of the next element is copied to the previous element
* <li>the original comment will be removed (either by traversing the children or by using cursor messages)
*
* @param <P> An input object that is passed to every visit method.
*/
@RequiredArgsConstructor
public class MergeYamlVisitor<P> extends YamlVisitor<P> {
Expand Down Expand Up @@ -88,8 +85,8 @@ public MergeYamlVisitor(Yaml scope, @Language("yml") String yamlString, boolean
.findFirst()
.map(Yaml.Documents.class::cast)
.map(docs -> {
// Any comments will have been put on the parent Document node, preserve by copying to the mapping
Document doc = docs.getDocuments().get(0);
// Any comments will have been put on the parent Yaml.Document node, preserve by copying to the mapping
Yaml.Document doc = docs.getDocuments().get(0);
if (doc.getBlock() instanceof Yaml.Mapping) {
Yaml.Mapping m = (Yaml.Mapping) doc.getBlock();
return m.withEntries(ListUtils.mapFirst(m.getEntries(), entry -> entry.withPrefix(doc.getPrefix())));
Expand Down Expand Up @@ -162,34 +159,41 @@ private boolean keyMatches(Yaml.Mapping m1, Yaml.Mapping m2) {
.orElse(false);
}

private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor cursor) {
private Yaml.Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor cursor) {
// Merge same key, different value together
List<Yaml.Mapping.Entry> mergedEntries = map(m1.getEntries(), existingEntry -> {
for (Entry incomingEntry : m2.getEntries()) {
for (Yaml.Mapping.Entry incomingEntry : m2.getEntries()) {
if (keyMatches(existingEntry, incomingEntry)) {
return existingEntry.withValue((Yaml.Block)
new MergeYamlVisitor<>(existingEntry.getValue(), incomingEntry.getValue(), acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
.visitNonNull(existingEntry.getValue(), p, new Cursor(cursor, existingEntry)));
Yaml.Block value = incomingEntry.getValue();
if (shouldAutoFormat && incomingEntry.getValue() instanceof Yaml.Scalar && hasLineBreak(((Yaml.Scalar) value).getValue())) {
Yaml.Block newValue = value.withMarkers(value.getMarkers().add(new MultilineScalarChanged(randomId(), false)));
value = autoFormat(newValue, p);
}
Yaml mergedYaml = new MergeYamlVisitor<>(existingEntry.getValue(), value, acceptTheirs, objectIdentifyingProperty, shouldAutoFormat)
.visitNonNull(existingEntry.getValue(), p, new Cursor(cursor, existingEntry));
return existingEntry.withValue((Yaml.Block) mergedYaml);
}
}
return existingEntry;
});

List<Entry> mutatedEntries = concatAll(mergedEntries, map(m2.getEntries(), it -> {
for (Entry existingEntry : m1.getEntries()) {
// Merge existing and new entries together
List<Yaml.Mapping.Entry> mutatedEntries = concatAll(mergedEntries, map(m2.getEntries(), it -> {
for (Yaml.Mapping.Entry existingEntry : m1.getEntries()) {
if (keyMatches(existingEntry, it)) {
return null;
}
}
// workaround: autoFormat cannot handle new inserted values very well
if (!mergedEntries.isEmpty() && it.getValue() instanceof Yaml.Scalar && hasLineBreak(mergedEntries.get(0), 2)) {
return it.withPrefix(linebreak() + grabAfterFirstLineBreak(mergedEntries.get(0)));
if (shouldAutoFormat && it.getValue() instanceof Yaml.Scalar && hasLineBreak(((Yaml.Scalar) it.getValue()).getValue())) {
it = it.withValue(it.getValue().withMarkers(it.getValue().getMarkers().add(new MultilineScalarChanged(randomId(), true))));
}
return shouldAutoFormat ? autoFormat(it, p, cursor) : it;
}));

// copy comment to previous element if needed
if (m1.getEntries().size() < mutatedEntries.size() && !getCursor().isRoot()) {
Cursor c = getCursor().dropParentUntil(it -> {
if (ROOT_VALUE.equals(it) || it instanceof Document) {
if (ROOT_VALUE.equals(it) || it instanceof Yaml.Document) {
return true;
}

Expand All @@ -205,11 +209,11 @@ private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor curso
return false;
});

if (c.getValue() instanceof Document || c.getValue() instanceof Yaml.Mapping) {
if (c.getValue() instanceof Yaml.Document || c.getValue() instanceof Yaml.Mapping) {
String comment = null;

if (c.getValue() instanceof Document) {
comment = ((Document) c.getValue()).getEnd().getPrefix();
if (c.getValue() instanceof Yaml.Document) {
comment = ((Yaml.Document) c.getValue()).getEnd().getPrefix();
} else {
List<Yaml.Mapping.Entry> entries = ((Yaml.Mapping) c.getValue()).getEntries();

Expand All @@ -221,7 +225,7 @@ private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor curso
}
}
// or retrieve it for last item from next element (could potentially be much higher in the tree)
if (comment == null && hasLineBreak(entries.get(entries.size() - 1), 1)) {
if (comment == null && hasLineBreak(entries.get(entries.size() - 1).getPrefix())) {
comment = grabBeforeFirstLineBreak(entries.get(entries.size() - 1));
}
}
Expand All @@ -242,7 +246,7 @@ private Mapping mergeMapping(Yaml.Mapping m1, Yaml.Mapping m2, P p, Cursor curso
private void removePrefixForDirectChildren(List<Yaml.Mapping.Entry> m1Entries, List<Yaml.Mapping.Entry> mutatedEntries) {
for (int i = 0; i < m1Entries.size() - 1; i++) {
if (m1Entries.get(i).getValue() instanceof Yaml.Mapping && mutatedEntries.get(i).getValue() instanceof Yaml.Mapping &&
((Yaml.Mapping) m1Entries.get(i).getValue()).getEntries().size() < ((Yaml.Mapping) mutatedEntries.get(i).getValue()).getEntries().size()) {
((Yaml.Mapping) m1Entries.get(i).getValue()).getEntries().size() < ((Yaml.Mapping) mutatedEntries.get(i).getValue()).getEntries().size()) {
mutatedEntries.set(i + 1, mutatedEntries.get(i + 1).withPrefix(linebreak() + grabAfterFirstLineBreak(mutatedEntries.get(i + 1))));
}
}
Expand Down Expand Up @@ -313,10 +317,6 @@ private Yaml.Sequence mergeSequence(Yaml.Sequence s1, Yaml.Sequence s2, P p, Cur
}
}

private boolean hasLineBreak(Yaml.Mapping.Entry entry, int atLeastParts) {
return LINE_BREAK.matcher(entry.getPrefix()).find() && LINE_BREAK.split(entry.getPrefix()).length >= atLeastParts;
}

private String grabBeforeFirstLineBreak(Yaml.Mapping.Entry entry) {
String[] parts = LINE_BREAK.split(entry.getPrefix());
return parts.length > 0 ? parts[0] : "";
Expand All @@ -327,7 +327,7 @@ private String grabAfterFirstLineBreak(Yaml.Mapping.Entry entry) {
return parts.length > 1 ? String.join(linebreak(), Arrays.copyOfRange(parts, 1, parts.length)) : "";
}

private Scalar mergeScalar(Yaml.Scalar y1, Yaml.Scalar y2) {
private Yaml.Scalar mergeScalar(Yaml.Scalar y1, Yaml.Scalar y2) {
String s1 = y1.getValue();
String s2 = y2.getValue();
return !s1.equals(s2) && !acceptTheirs ? y1.withValue(s2) : y1;
Expand Down
32 changes: 32 additions & 0 deletions rewrite-yaml/src/main/java/org/openrewrite/yaml/MultilineScalarChanged.java
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright 2024 the original author or authors.
* <p>
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
* <p>
* https://www.apache.org/licenses/LICENSE-2.0
* <p>
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package org.openrewrite.yaml;

import lombok.Value;
import lombok.With;
import org.openrewrite.marker.Marker;

import java.util.UUID;

/**
* Multiline scalars are added directly to the tree, which leads to a wrong ident level.
*/
@Value
@With
public class MultilineScalarChanged implements Marker {
UUID id;
boolean added;
}
24 changes: 18 additions & 6 deletions rewrite-yaml/src/main/java/org/openrewrite/yaml/format/IndentsVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import org.openrewrite.Cursor;
import org.openrewrite.Tree;
import org.openrewrite.internal.StringUtils;
import org.openrewrite.yaml.MultilineScalarChanged;
import org.openrewrite.yaml.YamlIsoVisitor;
import org.openrewrite.yaml.style.IndentsStyle;
import org.openrewrite.yaml.tree.Yaml;
Expand All @@ -29,6 +30,9 @@
import java.util.regex.Pattern;

public class IndentsVisitor<P> extends YamlIsoVisitor<P> {

private static final Pattern COMMENT_PATTERN = Pattern.compile("^(\\s*)(.*\\R?)", Pattern.MULTILINE);

private final IndentsStyle style;

@Nullable
Expand All @@ -46,7 +50,7 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {
Yaml y = c.getValue();
String prefix = y.getPrefix();

if (prefix.contains("\n")) {
if (StringUtils.hasLineBreak(prefix)) {
int indent = findIndent(prefix);
if (indent != 0) {
c.putMessage("lastIndent", indent);
Expand All @@ -68,7 +72,7 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {

Yaml y = tree;
int indent = getCursor().getNearestMessage("lastIndent", 0);
if (y.getPrefix().contains("\n") && !isUnindentedTopLevel()) {
if (StringUtils.hasLineBreak(y.getPrefix()) && !isUnindentedTopLevel()) {
if (y instanceof Yaml.Sequence.Entry) {
indent = getCursor().getParentOrThrow().getMessage("sequenceEntryIndent", indent);

Expand All @@ -94,7 +98,17 @@ public IndentsVisitor(IndentsStyle style, @Nullable Tree stopAfter) {
} else {
y = y.withPrefix(indentComments(y.getPrefix(), indent));
}
} else if (y instanceof Yaml.Scalar && y.getMarkers().findFirst(MultilineScalarChanged.class).isPresent()) {
int indentValue = indent;

if (!y.getMarkers().findFirst(MultilineScalarChanged.class).get().isAdded() && indent != 0) {
indentValue = indent + style.getIndentSize();
}

String newValue = ((Yaml.Scalar) y).getValue().replaceAll("\\R", "\n" + StringUtils.repeat(" ", indentValue));
y = ((Yaml.Scalar) y).withValue(newValue);
}

return y;
}

Expand All @@ -120,7 +134,7 @@ private boolean isUnindentedTopLevel() {
}

private String indentTo(String prefix, int column) {
if (prefix.contains("\n")) {
if (StringUtils.hasLineBreak(prefix)) {
int indent = findIndent(prefix);
if (indent != column) {
int shift = column - indent;
Expand All @@ -130,7 +144,6 @@ private String indentTo(String prefix, int column) {
return indentComments(prefix, column);
}

private static final Pattern COMMENT_PATTERN = Pattern.compile("^(\\s*)(.*\n?)", Pattern.MULTILINE);
private String indentComments(String prefix, int column) {
// If the prefix contains a newline followed by a comment ensure the comment begins at the indentation column
if (prefix.contains("#")) {
Expand All @@ -143,8 +156,7 @@ private String indentComments(String prefix, int column) {
String comment = m.group(2);
int newlineCount = StringUtils.countOccurrences(whitespace, "\n");
if (firstLine && newlineCount == 0) {
if(getCursor().getValue() instanceof Yaml.Documents ||
getCursor().getValue() instanceof Yaml.Document) {
if (getCursor().getValue() instanceof Yaml.Documents || getCursor().getValue() instanceof Yaml.Document) {
// Comments on a top-level
result.append(indent);
} else {
Expand Down
Loading

0 comments on commit 615a767

Please sign in to comment.