[MNG-8686] Add `SourceRoot.matcher(boolean)` method by desruisseaux · Pull Request #2236 · apache/maven

desruisseaux · 2025-04-05T12:49:44Z

Following the revert of includes/excludes filters from PathMatcher to String in #2232, this pull request adds a new SourceRoot.matcher(boolean) method which provides the PathMatcher. Contrarily to the previous methods, this new method provides a single matcher combining the work of all includes and all excludes. The default implementation is ported from apache/maven-clean-plugin#243.

JIRA issue: MNG-8686

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSourceRoot.java

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java

elharo

Does this have/need a JIRA issue?

api/maven-api-core/src/main/java/org/apache/maven/api/SourceRoot.java

impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultSourceRoot.java

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java

desruisseaux · 2025-04-12T21:20:28Z

JIRA issue created: https://issues.apache.org/jira/browse/MNG-8686

gnodet · 2025-04-13T08:48:01Z

Following the revert of includes/excludes filters from PathMatcher to String in #2232, this pull request adds a new SourceRoot.matcher(boolean) method which provides the PathMatcher. Contrarily to the previous methods, this new method provides a single matcher combining the work of all includes and all excludes. The default implementation is ported from apache/maven-clean-plugin#243.

We'd need to carefully review the maven-resources-plugins various options and see if they can fit this API. IIRC, there are options such as symlinks following, but there may be others.

desruisseaux · 2025-04-13T10:45:02Z

We'd need to carefully review the maven-resources-plugins various options and see if they can fit this API. IIRC, there are options such as symlinks following, but there may be others.

Yes. This is another aspect replaced by standard Java now, with java.nio.file.FileVisitOption.FOLLOW_LINKS. One issue is that it behaves differently than the current Maven for one kind of link specific to the Windows platform: NTFS "junction links". This is handled by a special case in apache/maven-clean-plugin#243. Thankfully, the maven-cleaner-plugin has a quite extensive test suite that covers this case among others.

Symbolic links are not handled by the PathSelector class proposed in this pull request. They need to be handled later, in our use of java.nio.file.FileVisitor, which is not part of this PR. I was thinking to address FileVisitor in a separated PR.

api/maven-api-core/src/main/java/org/apache/maven/api/SourceRoot.java

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java

gnodet · 2025-05-09T17:56:38Z

@desruisseaux can you rebase or merge master branch to fix the conflict ?

…), add a `SourceRoot.matcher(boolean)` method. The matcher returned by that method combines the effects of all includes and excludes. When using the Maven syntax, escape the special characters [ ] { } \ before to delegate to the glob syntax. Optimization: omit excludes that are unnecessary because they will never match a file accepted by includes. This is especially useful when the default excludes are added, because there is a lot of them.

desruisseaux · 2025-05-09T22:08:07Z

Rebasing done.

Pankraz76

polish

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

Pankraz76

consider Migrate JUnit asserts to AssertJ

Pankraz76 · 2025-05-11T08:48:17Z

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java

+        String s = matcher.toString();
+        assertTrue(s.contains("glob:**/*.java"));
+        assertFalse(s.contains("project.pj")); // Unnecessary exclusion should have been omitted.
+        assertFalse(s.contains(".DS_Store"));


Suggested change

assertFalse(s.contains(".DS_Store"));

assertThat(s).contains(".DS_Store");

new code, already, considered, outdated - therefore increasing technical dept.

to reach next lvl, we need leadership and team up to standardize DOD.

consider migration to avoid boilerplate API usage.

[POC] Migrate JUnit asserts to AssertJ - impl #2307

https://docs.openrewrite.org/recipes/java/testing/assertj/junittoassertj

Still new manual interaction due to bug:

assertThat(xml.contains("<name>Sample Plugin</name>")).isTrue(); openrewrite/rewrite-testing-frameworks#726

Let's do the refactoring in the other PR related to AssertJ migration.

this will serve our need:

https://docs.openrewrite.org/recipes/java/testing/junit5/junit5bestpractices

[POC] JUnit Jupiter best practices #2320

Co-authored-by: VIP <8830888+Pankraz76@users.noreply.github.com>

Pankraz76

warnings

Pankraz76 · 2025-05-11T21:52:14Z

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

+         * Note that a prefix or suffix may be the empty string, which match everything.
+         */
+        final Iterator<String> it = excludes.iterator();
+        nextExclude:


Unused label 'nextExclude'

Pankraz76 · 2025-05-11T21:53:18Z

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

+     * @return the given array, or a smaller array if some fragments were discarded because redundant
+     */
+    private static String[] sortByLength(final String[] fragments, final boolean suffix) {
+        Arrays.sort(fragments, (s1, s2) -> s1.length() - s2.length());


Can be replaced with 'Comparator.comparingInt'

Suggested change

Arrays.sort(fragments, (s1, s2) -> s1.length() - s2.length());

Arrays.sort(fragments, Comparator.comparingInt(String::length));

Pankraz76 · 2025-05-11T21:56:06Z

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

+     * <p><b>Source:</b> this list is copied from {@code plexus-utils-4.0.2} (released in
+     * September 23, 2024), class {@code org.codehaus.plexus.util.AbstractScanner}.</p>
+     */
+    private static final List<String> DEFAULT_EXCLUDES = List.of(


might use simple alphabetical order sorter like in .propertie files - to get sort and grouping out of the box:

The comments mostly DRY.

Suggested change

private static final List<String> DEFAULT_EXCLUDES = List.of(

private static final List<String> DEFAULT_EXCLUDES = List.of(

"**/*~",

"**/#*#",

"**/.#*",

"**/%*%",

"**/._*",

"**/CVS",

"**/CVS/**",

"**/.cvsignore",

"**/RCS",

"**/RCS/**",

"**/SCCS",

"**/SCCS/**",

"**/vssver.scc",

"**/project.pj",

"**/.svn",

"**/.svn/**",

"**/.arch-ids",

"**/.arch-ids/**",

"**/.bzr",

"**/.bzr/**",

"**/.MySCMServerInfo",

"**/.DS_Store",

"**/.metadata",

"**/.metadata/**",

"**/.hg",

"**/.hg/**",

"**/.git",

"**/.git/**",

"**/.gitignore",

"**/BitKeeper",

"**/BitKeeper/**",

"**/ChangeSet",

"**/ChangeSet/**",

"**/_darcs",

"**/_darcs/**",

"**/.darcsrepo",

"**/.darcsrepo/**",

"**/-darcs-backup*",

"**/.darcs-temp-mail"

);

same case: checkstyle/checkstyle#16760 (comment)

Pankraz76 · 2025-05-11T21:59:17Z

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java

+        if (baseDirectory.equals(directory)) {
+            return true;
+        }
+        directory = baseDirectory.relativize(directory);


smell never reassign params as we live in final land.

give dedication method and apply functional programming.

https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html#avoidreassigningparameters

public boolean couldHoldSelected(Path directory) { return baseDirectory.equals(directory) || isABoolean(baseDirectory.relativize(directory)); } private boolean isABoolean(Path directory) { return dirIncludes.length == 0 || isMatched(directory, dirIncludes) && (dirExcludes.length == 0 || !isMatched(directory, dirExcludes)); }

important AvoidReassigningParameters is of course part of bestpractices .

Still not encountered in

[POC-FIX-PMD] maven-pmd-plugin: reactivate PMD - Best practices #2331

Pankraz76

checkup

Pankraz76 · 2025-05-12T06:46:26Z

leveraging PMD will prevent flaws from being merged.

Pankraz76 · 2025-05-12T08:19:51Z

this could not compile with help of https://docs.openrewrite.org/recipes/staticanalysis/finalizemethodarguments

Pankraz76 · 2025-05-12T08:24:06Z

rewrite would make the boilerplate way i would prefer silent PMD.

https://adabeat.com/fp/immutability-in-functional-programming/

We make everywhere boilerplate to throw NPE ourselfs. This would really help to make thinks more robust. immutability is/should be the default case. Kotlin is all about being final and avoiding silly NPE. Thats why we want to live in final existence. Trying to reach invinity.

The matcher returned by that method combines the effects of all includes and excludes. When using the Maven syntax, escape the special characters [ ] { } \ before to delegate to the glob syntax. Optimization: omit excludes that are unnecessary because they will never match a file accepted by includes. This is especially useful when the default excludes are added, because there is a lot of them. --------- Co-authored-by: VIP <8830888+Pankraz76@users.noreply.github.com>

gnodet · 2025-05-12T16:26:29Z

rewrite would make the boilerplate way i would prefer silent PMD.

https://adabeat.com/fp/immutability-in-functional-programming/

We make everywhere boilerplate to throw NPE ourselfs. This would really help to make thinks more robust. immutability is/should be the default case. Kotlin is all about being final and avoiding silly NPE. Thats why we want to live in final existence. Trying to reach invinity.

I don't think we want additional reports. Or that would nice as a GitHub action that runs on PRs.
Additional reports without any automatic way to fix the issue on a big codebase it just useless.

Pankraz76 · 2025-05-12T18:20:46Z

useless

disagree, its called best practise for reason.

agree, with help of rewrite, we can get benefits for free. Enforcing really strong coding guidelines.

Pankraz76 · 2025-05-13T11:27:28Z

just need to use what we already got:

I see plugin in pom, but not running. So im this should be our SPOT meaning single point of truth.

https://pmd.github.io/pmd/pmd_rules_java_bestpractices.html

Then we dont have friction about final params. Im sure @desruisseaux very capable of, and would like to, following patterns to avoid bugs. We all want to best for the code.

gnodet · 2025-05-13T12:11:29Z

rewrite would make the boilerplate way i would prefer silent PMD.
https://adabeat.com/fp/immutability-in-functional-programming/
We make everywhere boilerplate to throw NPE ourselfs. This would really help to make thinks more robust. immutability is/should be the default case. Kotlin is all about being final and avoiding silly NPE. Thats why we want to live in final existence. Trying to reach invinity.

I don't think we want additional reports. Or that would nice as a GitHub action that runs on PRs. Additional reports without any automatic way to fix the issue on a big codebase it just useless.

Actually, they are available at https://maven.apache.org/ref/4.0.0-rc-3/maven-impl-modules/maven-core/pmd.html but I don't think we'll keep them on the long term.

Pankraz76 · 2025-05-13T12:34:14Z

long term

yes, rewrite has ability to dominate.

desruisseaux · 2025-05-13T14:47:31Z

If the discussion is about avoiding to reassign parameter values, those best practices are guidelines, often improving software but not always. For example, reassigning a parameter with something unrelated is dangerous. In that case, the best practice applies. But reassigning a parameter with a "fixed" (from the perspective of what the method wants to do) value can be good since it guarantees that the potentially broken value is never used. For example, if a method said that it ignores leading and trailing spaces, doing arg = arg.strip() at the beginning guarantees the "wrong" (from method's perspective) value with spaces will never be used.

Pankraz76 · 2025-05-13T15:30:36Z

not always

yes, will always allow suppression making exception, for good argument.

jira-importer · 2025-07-07T09:08:26Z

Resolve #9353

desruisseaux mentioned this pull request Apr 5, 2025

[MNG-8673] SourceRoot includes and excludes should be String #2232

Merged

elharo requested changes Apr 5, 2025

View reviewed changes

desruisseaux force-pushed the feat/selector branch from 1f5f25d to 1e1757c Compare April 5, 2025 14:38

elharo reviewed Apr 5, 2025

View reviewed changes

elharo approved these changes Apr 5, 2025

View reviewed changes

desruisseaux force-pushed the feat/selector branch from bb751b8 to 3522863 Compare April 5, 2025 15:32

desruisseaux changed the title ~~Add SourceRoot.matcher(boolean) method~~ [MNG-8686] Add SourceRoot.matcher(boolean) method Apr 12, 2025

elharo requested changes May 5, 2025

View reviewed changes

elharo reviewed May 6, 2025

View reviewed changes

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java Outdated Show resolved Hide resolved

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java Outdated Show resolved Hide resolved

elharo reviewed May 7, 2025

View reviewed changes

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java Outdated Show resolved Hide resolved

elharo approved these changes May 7, 2025

View reviewed changes

impl/maven-impl/src/test/java/org/apache/maven/impl/PathSelectorTest.java Outdated Show resolved Hide resolved

gnodet added this to the 4.0.0-rc-4 milestone May 9, 2025

gnodet approved these changes May 9, 2025

View reviewed changes

desruisseaux force-pushed the feat/selector branch from 1c028ec to fe0ca2f Compare May 9, 2025 21:19

Pankraz76 approved these changes May 10, 2025

View reviewed changes

impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java Outdated Show resolved Hide resolved

Pankraz76 suggested changes May 11, 2025

View reviewed changes

Pankraz76 mentioned this pull request May 11, 2025

[POC] Migrate JUnit asserts to AssertJ - impl #2307

Closed

Combine two if statements in a single one.

afe4179

Co-authored-by: VIP <8830888+Pankraz76@users.noreply.github.com>

Pankraz76 reviewed May 11, 2025

View reviewed changes

Pankraz76 suggested changes May 11, 2025

View reviewed changes

gnodet merged commit 75b6c11 into apache:master May 12, 2025
13 checks passed

github-actions bot assigned gnodet May 12, 2025

github-actions bot added the maintenance label May 12, 2025

Pankraz76 mentioned this pull request May 13, 2025

extract common functionality of DefaultPluginXmlFactory #2326

Closed

desruisseaux deleted the feat/selector branch May 13, 2025 17:48

Pankraz76 mentioned this pull request May 14, 2025

[POC-FIX-PMD] maven-pmd-plugin: reactivate PMD - Best practices #2331

Closed

	assertFalse(s.contains(".DS_Store"));
	assertThat(s).contains(".DS_Store");

	Arrays.sort(fragments, (s1, s2) -> s1.length() - s2.length());
	Arrays.sort(fragments, Comparator.comparingInt(String::length));

-    private static final List<String> DEFAULT_EXCLUDES = List.of(
+private static final List<String> DEFAULT_EXCLUDES = List.of(
+    "**/*~",
+    "**/#*#",
+    "**/.#*",
+    "**/%*%",
+    "**/._*",
+    "**/CVS",
+    "**/CVS/**",
+    "**/.cvsignore",
+    "**/RCS",
+    "**/RCS/**",
+    "**/SCCS",
+    "**/SCCS/**",
+    "**/vssver.scc",
+    "**/project.pj",
+    "**/.svn",
+    "**/.svn/**",
+    "**/.arch-ids",
+    "**/.arch-ids/**",
+    "**/.bzr",
+    "**/.bzr/**",
+    "**/.MySCMServerInfo",
+    "**/.DS_Store",
+    "**/.metadata",
+    "**/.metadata/**",
+    "**/.hg",
+    "**/.hg/**",
+    "**/.git",
+    "**/.git/**",
+    "**/.gitignore",
+    "**/BitKeeper",
+    "**/BitKeeper/**",
+    "**/ChangeSet",
+    "**/ChangeSet/**",
+    "**/_darcs",
+    "**/_darcs/**",
+    "**/.darcsrepo",
+    "**/.darcsrepo/**",
+    "**/-darcs-backup*",
+    "**/.darcs-temp-mail"
+);

Conversation

desruisseaux commented Apr 5, 2025 • edited by gnodet Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

elharo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

desruisseaux commented Apr 12, 2025

Uh oh!

gnodet commented Apr 13, 2025

Uh oh!

desruisseaux commented Apr 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gnodet commented May 9, 2025

Uh oh!

desruisseaux commented May 9, 2025

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnodet May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 left a comment

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Pankraz76 May 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

desruisseaux commented Apr 5, 2025 •

edited by gnodet

Loading

Pankraz76 May 11, 2025 •

edited

Loading

Pankraz76 May 12, 2025 •

edited

Loading

Pankraz76 May 11, 2025 •

edited

Loading

Pankraz76 May 11, 2025 •

edited

Loading

Pankraz76 commented May 13, 2025 •

edited

Loading

Pankraz76 commented May 13, 2025 •

edited

Loading