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.

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

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

Closed
Apache9 opened this issue Oct 1, 2022 · 4 comments
Closed
Labels

Comments

@Apache9
Copy link
Contributor

Apache9 commented Oct 1, 2022

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

if (name == null || searchRegex == null || replacement == null) {

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
Copy link
Member

nedtwigg commented Oct 2, 2022

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

if (name == null || search == null || replacement == null) {
throw new IllegalArgumentException("Must specify 'name', 'search' and 'replacement'.");
}

if (name == null || searchRegex == null || replacement == null) {
throw new IllegalArgumentException("Must specify 'name', 'searchRegex' and 'replacement'.");
}

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

@nedtwigg nedtwigg added the bug label Oct 2, 2022
@Apache9
Copy link
Contributor Author

Apache9 commented Oct 3, 2022

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]

@Apache9 Apache9 changed the title Allow replacement to be null for ReplaceRegex of plugin maven Allow replacement to be null for Replace and ReplaceRegex of plugin maven Oct 3, 2022
@nedtwigg
Copy link
Member

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

@Apache9
Copy link
Contributor Author

Apache9 commented Oct 10, 2022

Thanks @nedtwigg !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants