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

rework directory scanner to ensure it can enforce exclusions and it uses path #54

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

rmannibucau
Copy link

No description provided.

import java.io.File;

/**
* If an exclude is defined on a folder it will bypass the visit of the children
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will bypass --> bypasses

In general, tech writing prefers the present tense

better yet bypass the visit --> does not visit

@@ -243,14 +243,6 @@ public void testSimpleExcludes()
/* expExclDirs */ NONE );
}

public void testIsSymbolicLink()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this test removed?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No more relevant since there we use directly Files API and it was testing an internal

import org.junit.rules.TemporaryFolder;

import java.io.File;
import java.io.FileWriter;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

danger: this class depends on platform default encoding. Avoid.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is fine, it is just used to "touch" the file, not write content

@@ -33,9 +33,7 @@
* Significantly more efficient than using strings, since re-evaluation and re-tokenizing is avoided.
*
* @author Kristian Rosenvold
* @deprecated use {@code java.nio.filejava.nio.file.DirectoryStream.Filter<T>} and related classes
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please retain this

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this comment is wrong, it shouldn't have been merged IMO

@MartinKanters
Copy link

@rmannibucau Is this PR still valid and needed? We're looking to release maven-shared-utils and I'm wondering if we should include this.

@kwin
Copy link
Member

kwin commented Jun 2, 2021

IMHO this is relevant and would fix https://issues.apache.org/jira/browse/MSHARED-989.

* If an exclude is defined on a folder it does not visit of the children
* even if some include can match children.
*/
public class EnforceExcludesOverIncludes implements ScanConductor, ScannerAware
Copy link
Member

@kwin kwin Jun 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make sense if this class would directly implement https://docs.oracle.com/javase/7/docs/api/java/nio/file/DirectoryStream.Filter.html instead? That way one could get rid of the ScanConductor interface...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @kwin let's make it slim

@wilx
Copy link
Contributor

wilx commented Jun 6, 2022

Any progress?

@michael-o
Copy link
Member

@rmannibucau @kwin Do you guys want to pick this up? I would merge this into 4.0.0.

@kwin
Copy link
Member

kwin commented Jul 26, 2022

I don't have capacity for this unfortunately.

@michael-o
Copy link
Member

I don't have capacity for this unfortunately.

Alright, then I will remove the fix version and retain the class as-is for 4.0.0.

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

Successfully merging this pull request may close these issues.

6 participants