[MNG-8686] Add `SourceRoot.matcher(boolean)` method #2236

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.

The matcher returned by that method combines the effects of all includes and excludes.

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

elharo · 2025-04-05T13:21:45Z

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

@@ -144,13 +143,15 @@ public DefaultSourceRoot(final ProjectScope scope, final Language language, fina
     * @param scope scope of source code (main or test)
     * @param language language of the source code
     * @param directory directory of the source code
+     * @param includes list of patterns for the files to include, or {@code null} if unspecified


unspecified or nothing to include?

Unspecified. The default is plugin-dependent. For example, the compiler plugin will default to the *.java includes. Other plugins may have different default.

Effective Java suggests returning empty lists instead of nulls as a general practice.

Actually this "null or empty" (will update the documentation). This is a parameter rather than a return value, accepting both for convenience.

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

elharo · 2025-04-05T13:24:13Z

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

+     */
+    @Override
+    public String toString() {
+        var buffer = new StringBuilder();


personally I dislike var. Not sure if there's a project setting on this

I do not know if there is a Maven policy. Personally, I use it when it is redundant with what we can read on the same line with no possibility of different result (e.g., the result of new), but generally avoid it for the return value of a method call for example, as it is not obvious by reading the line.

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

elharo · 2025-04-05T14:53:31Z

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

@@ -144,13 +143,15 @@ public DefaultSourceRoot(final ProjectScope scope, final Language language, fina
     * @param scope scope of source code (main or test)
     * @param language language of the source code
     * @param directory directory of the source code
+     * @param includes list of patterns for the files to include, or {@code null} if unspecified


Effective Java suggests returning empty lists instead of nulls as a general practice.

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 · 2025-04-05T15:02:29Z

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

+     *
+     * @param path the path to test
+     */
+    private void assertContains(String path) {


I think this method would be clearer inlined. It's just one line.

That line has some degrees of freedom (the collection, the base directory, the message in case of failure), even if I agree that there is not much (yet) real freedom because there is only one collection and one directory. A purpose of using a method is that, when many calls to assertContains are repeated, we know that those things are the same for every lines without having to paid a closer look to the lines.

I will rename as assertFilteredFilesContains for more clarity.

by `PathMatcher matcher(Collection, boolean)`.

…more because of the introduction or more argument to the `matcher` method in previous commit.

…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-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.

gnodet and others added 3 commits April 4, 2025 08:40

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

1326dbe

Improve javadoc

08a7643

Add a SourceRoot.matcher(boolean) method.

10d7d79

The matcher returned by that method combines the effects of all includes and excludes.

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

Add documentation.

1e1757c

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

More corrections in documentation.

3522863

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

desruisseaux added 4 commits April 5, 2025 18:06

Replace Optional<PathMatcher> matcher(boolean)

44921f3

by `PathMatcher matcher(Collection, boolean)`.

Fix a javadoc error.

47597ba

Remove the cache of PathMatcher. The could not be easily cached any…

a5125c8

…more because of the introduction or more argument to the `matcher` method in previous commit.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MNG-8686] Add `SourceRoot.matcher(boolean)` method #2236

[MNG-8686] Add `SourceRoot.matcher(boolean)` method #2236

desruisseaux commented Apr 5, 2025

elharo Apr 5, 2025

desruisseaux Apr 5, 2025

elharo Apr 5, 2025

desruisseaux Apr 5, 2025

elharo Apr 5, 2025

desruisseaux Apr 5, 2025

elharo left a comment

elharo Apr 5, 2025

elharo Apr 5, 2025

desruisseaux Apr 5, 2025

desruisseaux commented Apr 12, 2025

gnodet commented Apr 13, 2025

desruisseaux commented Apr 13, 2025

[MNG-8686] Add SourceRoot.matcher(boolean) method #2236

Are you sure you want to change the base?

[MNG-8686] Add SourceRoot.matcher(boolean) method #2236

Conversation

desruisseaux commented Apr 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desruisseaux commented Apr 12, 2025

gnodet commented Apr 13, 2025

desruisseaux commented Apr 13, 2025

[MNG-8686] Add `SourceRoot.matcher(boolean)` method #2236

[MNG-8686] Add `SourceRoot.matcher(boolean)` method #2236