Skip to content

Commit

Permalink
Remove maven properties should remove comment (#4554)
Browse files Browse the repository at this point in the history 
* Remove maven properties should remove comment

* Optionally remove preceding Xml comments too

* Merge nested if statements

* Remove preceding comments by default, except in AddProfile

---------

Co-authored-by: Tim te Beek <tim@moderne.io>
  • Loading branch information
@jonesbusy @timtebeek
jonesbusy and timtebeek authored Oct 6, 2024
1 parent d63ca04 commit 094e714
Show file tree
Hide file tree
Showing 17 changed files with 142 additions and 27 deletions.
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/AddProfile.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (maybeProfile.isPresent()) {
Xml.Tag profile = maybeProfile.get();

t = (Xml.Tag) new RemoveContentVisitor(profile, false).visitNonNull(t, ctx, getCursor().getParentOrThrow());
t = (Xml.Tag) new RemoveContentVisitor(profile, false, false).visitNonNull(t, ctx, getCursor().getParentOrThrow());

}
Xml.Tag profileTag = Xml.Tag.build("<profile>\n" +
Expand Down
4 changes: 2 additions & 2 deletions rewrite-maven/src/main/java/org/openrewrite/maven/AddRepository.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
repositories = (Xml.Tag) new AddToTagVisitor<>(repo, Xml.Tag.build(assembleReleases()))
.visitNonNull(repositories, ctx, getCursor().getParentOrThrow());
} else {
repositories = (Xml.Tag) new RemoveContentVisitor<>(releases, true)
repositories = (Xml.Tag) new RemoveContentVisitor<>(releases, true, true)
.visitNonNull(repositories, ctx, getCursor().getParentOrThrow());
if (!isNoSnapshots()) {
repositories = (Xml.Tag) new AddToTagVisitor<>(repo, Xml.Tag.build(assembleReleases()))
Expand All @@ -184,7 +184,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (snapshots == null) {
repositories = (Xml.Tag) new AddToTagVisitor<>(repo, Xml.Tag.build(assembleSnapshots())).visitNonNull(repositories, ctx, getCursor().getParentOrThrow());
} else {
repositories = (Xml.Tag) new RemoveContentVisitor<>(snapshots, true).visitNonNull(repositories, ctx, getCursor().getParentOrThrow());
repositories = (Xml.Tag) new RemoveContentVisitor<>(snapshots, true, true).visitNonNull(repositories, ctx, getCursor().getParentOrThrow());
if (!isNoSnapshots()) {
repositories = (Xml.Tag) new AddToTagVisitor<>(repo, Xml.Tag.build(assembleSnapshots())).visitNonNull(repositories, ctx, getCursor().getParentOrThrow());
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyClassifier.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
Optional<Xml.Tag> classifier = tag.getChild("classifier");
if (classifier.isPresent()) {
if (newClassifier == null) {
doAfterVisit(new RemoveContentVisitor<>(classifier.get(), false));
doAfterVisit(new RemoveContentVisitor<>(classifier.get(), false, true));
} else if (!newClassifier.equals(classifier.get().getValue().orElse(null))) {
doAfterVisit(new ChangeTagValueVisitor<>(classifier.get(), newClassifier));
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyGroupIdAndArtifactId.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (versionTagPresent) {
// If the previous dependency had a version but the new artifact is managed, removed the version tag.
if (!configuredToOverrideManageVersion && newDependencyManaged || (oldDependencyManaged && configuredToChangeManagedDependency)) {
t = (Xml.Tag) new RemoveContentVisitor<>(versionTag.get(), false).visit(t, ctx);
t = (Xml.Tag) new RemoveContentVisitor<>(versionTag.get(), false, true).visit(t, ctx);
} else {
// Otherwise, change the version to the new value.
t = changeChildTagValue(t, "version", resolvedNewVersion, ctx);
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/ChangeDependencyScope.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
Optional<Xml.Tag> scope = tag.getChild("scope");
if (scope.isPresent()) {
if (newScope == null) {
doAfterVisit(new RemoveContentVisitor<>(scope.get(), false));
doAfterVisit(new RemoveContentVisitor<>(scope.get(), false, true));
} else if (!newScope.equals(scope.get().getValue().orElse(null))) {
doAfterVisit(new ChangeTagValueVisitor<>(scope.get(), newScope));
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/ManageDependencies.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ public RemoveVersionTagVisitor(String groupPattern, String artifactPattern) {
@Override
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (isDependencyTag() && isDependencyTag(groupPattern, artifactPattern)) {
tag.getChild("version").ifPresent(versionTag -> doAfterVisit(new RemoveContentVisitor<>(versionTag, false)));
tag.getChild("version").ifPresent(versionTag -> doAfterVisit(new RemoveContentVisitor<>(versionTag, false, true)));
return tag;
}
return super.visitTag(tag, ctx);
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/RemoveDependency.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
Scope checkScope = scope != null ? Scope.fromName(scope) : null;
ResolvedDependency dependency = findDependency(tag, checkScope);
if (dependency != null) {
doAfterVisit(new RemoveContentVisitor<>(tag, true));
doAfterVisit(new RemoveContentVisitor<>(tag, true, true));
maybeUpdateModel();
}
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/RemoveManagedDependency.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
Scope checkScope = scope != null ? Scope.fromName(scope) : null;
boolean isBomImport = tag.getChildValue("scope").map("import"::equalsIgnoreCase).orElse(false);
if (isBomImport || findManagedDependency(tag, checkScope) != null) {
doAfterVisit(new RemoveContentVisitor<>(tag, true));
doAfterVisit(new RemoveContentVisitor<>(tag, true, true));
maybeUpdateModel();
}
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/RemovePlugin.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
@Override
public Xml.Document visitDocument(Xml.Document document, ExecutionContext ctx) {
for (Xml.Tag plugin : FindPlugin.find(document, groupId, artifactId)) {
doAfterVisit(new RemoveContentVisitor<>(plugin, true));
doAfterVisit(new RemoveContentVisitor<>(plugin, true, true));
}
return super.visitDocument(document, ctx);
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-maven/src/main/java/org/openrewrite/maven/RemoveProperty.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ private class RemovePropertyVisitor extends MavenVisitor<ExecutionContext> {
@Override
public Xml visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (isPropertyTag() && propertyName.equals(tag.getName())) {
doAfterVisit(new RemoveContentVisitor<>(tag, true));
doAfterVisit(new RemoveContentVisitor<>(tag, true, true));
maybeUpdateModel();
}
return super.visitTag(tag, ctx);
Expand Down
2 changes: 1 addition & 1 deletion ...ain/java/org/openrewrite/maven/cleanup/DependencyManagementDependencyRequiresVersion.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
@Override
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (isManagedDependencyTag() && tag.getChildValue("version").orElse(null) == null) {
doAfterVisit(new RemoveContentVisitor<>(tag, true));
doAfterVisit(new RemoveContentVisitor<>(tag, true, true));
}
return super.visitTag(tag, ctx);
}
Expand Down
2 changes: 2 additions & 0 deletions rewrite-maven/src/test/java/org/openrewrite/maven/AddProfileTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ void preExistingMatchingProfile() {
<artifactId>artifact</artifactId>
<version>1</version>
<profiles>
<!-- retained comment -->
<profile>
<id>myprofile</id>
<activation>
Expand All @@ -146,6 +147,7 @@ void preExistingMatchingProfile() {
<artifactId>artifact</artifactId>
<version>1</version>
<profiles>
<!-- retained comment -->
<profile>
<id>myprofile</id>
<activation>
Expand Down
120 changes: 113 additions & 7 deletions rewrite-maven/src/test/java/org/openrewrite/maven/RemovePropertyTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@ void removeProperty() {
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<a.version>a</a.version>
<bla.version>b</bla.version>
Expand All @@ -55,11 +55,11 @@ void removeProperty() {
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<a.version>a</a.version>
</properties>
Expand All @@ -83,11 +83,11 @@ void removeOnlyProperty() {
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<bla.version>b</bla.version>
</properties>
Expand All @@ -96,7 +96,7 @@ void removeOnlyProperty() {
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
Expand All @@ -111,4 +111,110 @@ void removeOnlyProperty() {
)
);
}

@Test
void removePropertyWithComment() {
rewriteRun(
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<a.version>a</a.version>
<!-- I should remove this property -->
<bla.version>b</bla.version>
</properties>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<a.version>a</a.version>
</properties>
</project>
""",
sourceSpecs ->
sourceSpecs.afterRecipe(d -> {
MavenResolutionResult resolution = d.getMarkers().findFirst(MavenResolutionResult.class).orElseThrow();
Map<String, String> properties = resolution.getPom().getRequested().getProperties();
assertThat(properties.get("a.version")).isEqualTo("a");
assertThat(properties.get("bla.version")).isNull();
})
)
);
}

@Test
void removePropertyWithCommentAndEmptyParents() {
rewriteRun(
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<!-- I should remove this property -->
<bla.version>b</bla.version>
</properties>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
</project>
"""
)
);
}

@Test
void removePropertyWithTwoComments() {
rewriteRun(
pomXml(
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
<properties>
<!-- And also remove this comment -->
<!-- I should remove this property -->
<bla.version>b</bla.version>
</properties>
</project>
""",
"""
<project>
<modelVersion>4.0.0</modelVersion>
<groupId>com.mycompany.app</groupId>
<artifactId>my-app</artifactId>
<version>1</version>
</project>
"""
)
);
}
}
13 changes: 10 additions & 3 deletions rewrite-xml/src/main/java/org/openrewrite/xml/RemoveContentVisitor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,12 @@ public class RemoveContentVisitor<P> extends XmlVisitor<P> {

private final Content scope;
private final boolean removeEmptyAncestors;
private final boolean removePrecedingComment;

public RemoveContentVisitor(Content tag, boolean removeEmptyAncestors) {
public RemoveContentVisitor(Content tag, boolean removeEmptyAncestors, boolean removePrecedingComment) {
this.scope = tag;
this.removeEmptyAncestors = removeEmptyAncestors;
this.removePrecedingComment = removePrecedingComment;
}

@Override
Expand All @@ -39,13 +41,18 @@ public Xml visitTag(Xml.Tag tag, P p) {
for (Content content : t.getContent()) {
if (scope.isScope(content)) {
List<Content> contents = new ArrayList<>(t.getContent());
contents.remove(content);
int indexOf = contents.indexOf(content);
contents.remove(indexOf);

if (removePrecedingComment && 0 < indexOf && contents.get(indexOf - 1) instanceof Xml.Comment) {
doAfterVisit(new RemoveContentVisitor<>(contents.get(indexOf - 1), true, removePrecedingComment));
}

if (removeEmptyAncestors && contents.isEmpty() && t.getAttributes().isEmpty()) {
if (getCursor().getParentOrThrow().getValue() instanceof Xml.Document) {
return t.withContent(null).withClosing(null);
} else {
doAfterVisit(new RemoveContentVisitor<>(t, true));
doAfterVisit(new RemoveContentVisitor<>(t, true, removePrecedingComment));
}
} else {
return t.withContent(contents);
Expand Down
2 changes: 1 addition & 1 deletion rewrite-xml/src/main/java/org/openrewrite/xml/RemoveEmptyXmlTags.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
Xml.Tag t = super.visitTag(tag, ctx);
//noinspection ConstantValue
if (t != null && (t.getContent() == null || t.getContent().isEmpty()) && t.getAttributes().isEmpty()) {
doAfterVisit(new RemoveContentVisitor<>(t, true));
doAfterVisit(new RemoveContentVisitor<>(t, true, true));
}
return t;
}
Expand Down
2 changes: 1 addition & 1 deletion rewrite-xml/src/main/java/org/openrewrite/xml/RemoveXmlTag.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
@Override
public Xml.Tag visitTag(Xml.Tag tag, ExecutionContext ctx) {
if (xPathMatcher.matches(getCursor())) {
doAfterVisit(new RemoveContentVisitor<>(tag, true));
doAfterVisit(new RemoveContentVisitor<>(tag, true, true));
}
return super.visitTag(tag, ctx);
}
Expand Down
6 changes: 3 additions & 3 deletions rewrite-xml/src/test/java/org/openrewrite/xml/RemoveContentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ void removeContent() {
spec -> spec.recipe(toRecipe(() -> new XmlVisitor<>() {
@Override
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
doAfterVisit(new RemoveContentVisitor<>(requireNonNull(x.getRoot().getContent()).get(1), false));
doAfterVisit(new RemoveContentVisitor<>(requireNonNull(x.getRoot().getContent()).get(1), false, true));
return super.visitDocument(x, ctx);
}
}).withMaxCycles(1)),
Expand All @@ -61,7 +61,7 @@ void removeAncestorsThatBecomeEmpty() {
@Override
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
doAfterVisit(new RemoveContentVisitor<>(requireNonNull(x.getRoot().getChildren()).get(1)
.getChildren().get(0).getChildren().get(0), true));
.getChildren().get(0).getChildren().get(0), true, true));
return super.visitDocument(x, ctx);
}
}).withMaxCycles(1)),
Expand Down Expand Up @@ -92,7 +92,7 @@ void rootChangedToEmptyTagIfLastRemainingTag() {
@Override
public Xml visitDocument(Xml.Document x, ExecutionContext ctx) {
doAfterVisit(new RemoveContentVisitor<>(requireNonNull(x.getRoot().getChildren()).get(0)
.getChildren().get(0).getChildren().get(0), true));
.getChildren().get(0).getChildren().get(0), true, true));
return super.visitDocument(x, ctx);
}
}).withMaxCycles(1)),
Expand Down

0 comments on commit 094e714

Please sign in to comment.