Add PathMatcherFactory.includesAll() #10964
Conversation
* Simplify the patch matcher when possible. * Add a `PathMatcherFactory.includesAll()` method. The two changes, combined together, allows the caller to know when a matcher include all files. The Maven Clean Plugin needs this information for checking if it can run in a background thread.
api/maven-api-core/src/main/java/org/apache/maven/api/services/PathMatcherFactory.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultPathMatcherFactory.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/services/PathMatcherFactory.java
Outdated
Show resolved
Hide resolved
| * It should be the matcher returned by the other methods of this interface when the | ||
| * given patterns match all files. Therefore, the following idiom can be used: | ||
| * | ||
| * <pre>PathMatcher fileMatcher = factory.createPathMatcher(dir, includes, excludes); |
There was a problem hiding this comment.
This strikes me as extremely brittle. Need to dig into the code more, but this makes me nervous. I'm not sure we can count on this. There are no guarantees on the behavior of simplify, and making guarantees might be solving the halting problem.
There was a problem hiding this comment.
simplify() is specific to the implementation (it is not a method of the PathMatcher interface) and will probably go away if we apply the plan in above comment. In the current code, this is safe because the implementation knows what it is doing. We could also add a PathMatcherFactory.isIncludesAll(PathPatcher) method, but it would not be safer compared to the current code. It may be clearer however.
When testing whether a path matcher includes all files, it is okay to have false negatives as the only consequence (at least in Maven Clean Plugin) will be loosing optimization opportunities. It is important to not have false positives however, but the current proposal mets that requirement.
There was a problem hiding this comment.
I'm not yet convinced it does guarantee no false positives. That depends on the implementation of the simplify method which doesn't seem to be enforced.
There was a problem hiding this comment.
The implementation of simplify() is enforced. This method exists only on PathSelector, this is not a Java API.
There was a problem hiding this comment.
! added an isIncludesAll(PathMatcher) method for clarity and applied @gnodet suggestion: make the PathSelector constructor and simplify() method private, replaced by a static of method, for making clearer what is under our control.
Make the `PathSelector` constructor and `simplify()` method private for enforcing the assumption behind `isIncludesAll` implementation.
api/maven-api-core/src/main/java/org/apache/maven/api/services/PathMatcherFactory.java
Outdated
Show resolved
Hide resolved
impl/maven-impl/src/main/java/org/apache/maven/impl/PathSelector.java
Outdated
Show resolved
Hide resolved
Resolved conflicts by accepting deletion of documentation files that were moved/reorganized in master: - src/site/markdown/configuration.properties - src/site/markdown/configuration.yaml - src/site/markdown/maven-configuration.md These files were moved to src/configuration-templates/ in master. Merged changes include: - Fix doap_Maven.rdf - Add PathMatcherFactory.includesAll() (apache#10964) - Generating configuration documentation during site build (apache#10961) - Use correct namespace in settings.xml (apache#10974) - Make error message less awkward (apache#10953) All plugin migration changes preserved and compatible with latest master.
* Minor modifications to the `PathMatcherFactor` service: * Make package-private. * Ensure that `simplify()` is always invoked. * Add `PathMatcherFactory.includesAll()` and `isIncludesAll(PathMatcher)` methods. The Maven Clean Plugin needs this information for checking if it can run in a background thread.
Minor modifications to the `PathMatcherFactor` service: * Make package-private. * Ensure that `simplify()` is always invoked. * Add `PathMatcherFactory.includesAll()` and `isIncludesAll(PathMatcher)` methods. The Maven Clean Plugin needs this information for checking if it can run in a background thread.
3 participants
Minor modifications to the
PathMatcherFactoryservice:PathMatcherFactory.includesAll()method.The two changes, combined together, allows the caller to know when a matcher includes all files. The Maven Clean Plugin needs this information for deciding if it can delete the files in a background thread.