Сhanged the behavior of the RemoveProperty recipe

[mankoffs]

What version of OpenRewrite are you using?

I am using

• rewrite-recipe-bom v2.21.1
• rewrite-maven-plugin v5.43.1
• rewrite-xml-module v8.38.0

How are you running OpenRewrite?

I'm running the recipe org.openrewrite.maven.RemoveProperty
new RemoveProperty("prop1")

What is the smallest, simplest way to reproduce the problem?

<project>                                                                                                  
<groupId>com.mycompany.app</groupId>                                                                   
<artifactId>project</artifactId>                                                                       
<version>1</version>                                                                                   
                                                                                                                                           
<properties>                                                                                           
    <service.name>test-service</service.name>                                                          
    <!-- My comment -->                                                   
    <prop1>prop</prop1>  
</properties>

What did you expect to see?

<project>                                                                                                  
<groupId>com.mycompany.app</groupId>                                                                   
<artifactId>project</artifactId>                                                                       
<version>1</version>                                                                                   
                                                                                                                                           
<properties>                                                                                           
    <service.name>test-service</service.name>                                                          
    <!-- My comment -->                                                   
</properties>

What did you see instead?

<project>                                                                                                  
<groupId>com.mycompany.app</groupId>                                                                   
<artifactId>project</artifactId>                                                                       
<version>1</version>                                                                                   
                                                                                                                                           
<properties>                                                                                           
    <service.name>test-service</service.name>                                                          
</properties>

Hello!
The problem is that the behavior of the RemoveContentVisitor method has changed, a property has been added there (removePrecedingComment), and this property is set as "true" by defalut. This behavior breaks recipes, this property should be configured in RemoveProperty.

[timtebeek]

hi @mankoffs , welcome back! Thanks for linking back to that earlier work, we did indeed change the default in

• Remove maven properties should remove comment #4554

We consider this new default to be the least surprising to users, but there is of course chance that a comment would be removed that you'd have rather retained. Perhaps for those cases you can provide your own recipe that copies these lines but retains the comment by overriding the default:

rewrite/rewrite-maven/src/main/java/org/openrewrite/maven/RemoveProperty.java

Lines 51 to 60 in d3c231d

	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, true));
	maybeUpdateModel();
	}
	return super.visitTag(tag, ctx);
	}
	}

We try not to replicate these options across all recipes, as otherwise each recipe touched in #4554 would get a new optional argument; that's perhaps a bit much for a handful lines of code.

I hope that gives you enough of a way forward; I'll monitor replies here in case there's a convincing case to change things still, but otherwise consider this issues as not directly actionable, as the default indeed changed.