diffplug / spotless

Keep your code spotless
Apache License 2.0
4.51k stars 455 forks source link

Allow replacement to be null for Replace and ReplaceRegex of plugin maven #1359

Closed Apache9 closed 2 years ago

Apache9 commented 2 years ago

In HBase, we want to purge useless javadoc tags and this is what currently we have done

https://github.com/apache/hbase/blob/23a56331dbeaa2016a27abbd1c094ce88bcf966a/pom.xml#L2737

            <!--
              e.g., remove the following lines:
              "* @param paramName"
              "* @throws ExceptionType"
              "* @return returnType"'
              Multiline to allow anchors on newlines
            -->
            <replaceRegex>
              <name>Remove unhelpful javadoc stubs</name>
              <searchRegex>(?m)^ *\* *@(?:param|throws|return) *\w* *\n</searchRegex>
              <!-- spotless manve plugin does not allow empty here, so use \n -->
              <replacement>\n</replacement>
            </replaceRegex>

Actually this is copied and modified from a gradle project

https://apache.googlesource.com/geode/+/refs/tags/rel/v1.11.0/gradle/spotless.gradle

    custom 'Remove unhelpful javadoc stubs', {
      // e.g., remove the following lines:
      // "* @param paramName"
      // "* @throws ExceptionType"
      // "* @return returnType"'
      // Multiline to allow anchors on newlines
      it.replaceAll(/(?m)^ *\* *@(?:param|throws|return) *\w* *\n/, '')
    }
    custom 'Remove any empty Javadocs and block comments', {
      // Matches any /** [...] */ or /* [...] */ that contains:
      // (a) only whitespace
      // (b) trivial information, such as "@param paramName" or @throws ExceptionType
      //     without any additional information.  This information is implicit in the signature.
      it.replaceAll(/\/\*+\s*\n(\s*\*\s*\n)*\s*\*+\/\s*\n/, '')
    }

You can see that we use '\n' instead a empty string because maven will consider the option is not provided if we use empty string and because of the check in this line

https://github.com/diffplug/spotless/blob/7667a849d377b546d45350b77b65e0793e3d17dc/plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/ReplaceRegex.java#L38

The spotless:apply execution will fail.

But actually, the '\n' does not work, spotless:apply will ignore the '\' and leave a 'n' there...

Before

  /**
   * @param zkData
   */

After spotless:apply

  /**
   * n
   */

I've opened an issue in HBase, for solving this problem https://issues.apache.org/jira/browse/HBASE-27400 But till now I couldn't find a proper way...

So I wonder if we could change the implementation of ReplaceRegex, to not force replacement to be non-null. If it is null, we just use an empty string as the replacement.

WDYT?

Thanks.

nedtwigg commented 2 years ago

That's a great idea! Happy to merge a PR for this. I think these are the relevant places

https://github.com/diffplug/spotless/blob/7667a849d377b546d45350b77b65e0793e3d17dc/plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/Replace.java#L38-L40

https://github.com/diffplug/spotless/blob/7667a849d377b546d45350b77b65e0793e3d17dc/plugin-maven/src/main/java/com/diffplug/spotless/maven/generic/ReplaceRegex.java#L38-L40

As a workaround, you can try XML escape sequences like &#10;

Apache9 commented 2 years ago

Thanks for the quick response. Will prepare a PR soon.

As a workaround, you can try XML escape sequences like

Sadly this does not work... Maven will still trim it and will get the same error when running spotless:apply...

[ERROR] Failed to execute goal com.diffplug.spotless:spotless-maven-plugin:2.24.1:apply (default-cli) on project hbase: Execution default-cli of goal com.diffplug.spotless:spotless-maven-plugin:2.24.1:apply failed: Must specify 'name', 'searchRegex' and 'replacement'. -> [Help 1]
nedtwigg commented 2 years ago

Fixed in plugin-maven 2.27.2, thanks to a PR from @Apache9.

Apache9 commented 2 years ago

Thanks @nedtwigg !