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

RAT-322: add option to be able to scan hidden directories and minor cleanups #160

Closed
wants to merge 1 commit into from

Conversation

jbonofre
Copy link
Member

  • add support to scan hidden directories flag to DirectoryWalker
  • add scan hidden directories to the ReportConfiguration
  • cleanup Walker moving isRestricted to the DirectoryWalker as it's specific
  • expose scan hidden directories configuration in CLI, Ant task and Maven plugin

@jbonofre
Copy link
Member Author

@ottlinger As discussed in Jira 😄

@ottlinger
Copy link
Contributor

@Claudenw is this in sync with what you proposed in RAT-323?

@jbonofre
Copy link
Member Author

I quickly discussed with @Claudenw on Slack. I think it's aligned.
As I detected issue in missing ASF headers in files located in hidden directories, it would be great to release a new rat version including this improvement.

@ottlinger
Copy link
Contributor

@jbonofre can you add an entry in the changelog? Thanks

@jbonofre
Copy link
Member Author

@ottlinger sure, I will :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the DirectoryWalker should accept a second FilenameFIlter rather than a boolean to limit the files, similar to the way the files are filtered. This will give us more control over what files to read -- I see this as being necessary for some of the harmonization work that is coming. Specifically reading specific hidden directories.

This change means that the configuration needs a pair of methods to set and get the input directory filter.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure: I think it's already covered by the excludes. If we are able to read all directories (including the hidden files), then we can filter with exclude.
I think introducing a filter for "include" would be confusing. I would rather read all directories and delegate to exclude.
A valid proposal would be to scan all directories by default (including hidden ones) and use excludes.
Thoughts ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree. However, if we switch to org.apache.commons.io.filefilter.AbstractFileFilter we can merge multiple filters together.

What we can do is modify the ReportConfiguration so that

  • by default set the directory filter to ignore dot prefixed directories.
  • if addDirectoryFilter() is called with NULL we remove the directory filter.
  • If addDirectoryFilter() is called with a non-null argument then the argument is "and"ed to the filter.

Alternatively we could create a FilterBuilder that constructs filters using "and" and "or" methods to build an AbstractFileFilter and accept the builder as the argument for a 'setDirectoryFilter()' method. Then passing NULL could set the internal value to org.apache.commons.io.filefilter.TrueFileFilter (accepting all directories).

I think that creating a builder will make specifying options in ANT UI easier. However, for now we can make due with adding a directoryFilter instance variable of type org.apache.commons.io.filefilter.AbstractFileFilter and setting it to ignore dot prefixed files by default. Then your current method can simpley change that to org.apache.commons.io.filefilter.TrueFileFilter. Later we can figure out how to specify more complex filters.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you add the AbstractFileFilter then it is probably advisable to change the internal representation of ReportConfiguration.inputFileFilter to that type as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, let me update the PR then. Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I experimented using IOFileFilter (leveraging HiddenFileFilter, FalseFileFilter, etc).
Adding addDirectoryFilter() to ReportConfiguration is OK (and using IOFileFIlter#and() to add multiple filters.

At code level, it works fine, but I have a concern about the "usage", especially on the CLI and Maven. I think for the CLI, we should keep the proposed option (--scan-hidden-directories for CLI for instance) to simplify the use.

Thoughts ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I opened #166 using DirectoryFilter. @Claudenw @ottlinger let me know which approach you prefer :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think my approach would be adding the addDirectoryFilter to the ReportConfiguration so we have the groundwork for the other cases.

Then add the --scan-hidden-directories option (or equivalent) to all the clients. (CLI, Maven, ANT). Making sure the --scan-hidden-directories is implemented across all the clients is part of RAT-323.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree, it's what I did in the other PR. I can cancel this one in favor of the new one. Can you please take a look on the new PR ?

@Claudenw
Copy link
Contributor

Thank you for the DirectoryWalkerTest.

@ottlinger
Copy link
Contributor

@jbonofre can we close this MR here as discussions are continuing in #166 (just to not confuse any reviewer/reader). Thanks for your contributions.

@jbonofre
Copy link
Member Author

@ottlinger absolutely :)

@jbonofre jbonofre closed this Nov 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants