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: be able to scan hidden directories #166

Merged
merged 1 commit into from
Nov 29, 2023

Conversation

jbonofre
Copy link
Member

This is an alternative to #160

@Claudenw
Copy link
Contributor

This looks good to me, but I have not built and tested it. I trust that the build works and there seems to be a test so I approve this change.

@jbonofre
Copy link
Member Author

@Claudenw I'm checking the build failure on Windows (Jenkins). I tested locally without problem. I'm doing a new pass and I close the "old" PR on the same topic.

@jbonofre
Copy link
Member Author

OK, I understand: on Windows, the HiddenFileFilter doesn't actually consider file hidden, so the tests are failing on Windows. I'm checking this part.

@jbonofre
Copy link
Member Author

Extracted from java.io.File#isHidden() javadoc:

Tests whether the file named by this abstract pathname is a hidden file. The exact definition of hidden is system-dependent. On UNIX systems, a file is considered to be hidden if its name begins with a period character ('.'). On Microsoft Windows systems, a file is considered to be hidden if it has been marked as such in the filesystem.

So, for the tests, I have to bypass if on Windows. I'm doing that.

@jbonofre jbonofre force-pushed the RAT-322-2 branch 4 times, most recently from 1ac5866 to 785c968 Compare November 25, 2023 06:10
@jbonofre
Copy link
Member Author

In order to be backward compatible (especially to work as before on Windows), I created NameBasedHiddenFileFilter that only check if file name contains . (and not the hidden attribute on Windows).
User can still define its own filter if needed.

@ottlinger
Copy link
Contributor

Thanks for the contribution @jbonofre

I'm still curious to get to know more details about why you want to scan in hidden directories? Can you provide more info about that.

@ottlinger
Copy link
Contributor

@Claudenw could we just merge this change or would this introduce problems with other/existing PRs? Thanks

@ottlinger
Copy link
Contributor

@jbonofre feel free to add a changelog entry (or I can do this upon merging your changes; sorry for my many questions in the code).

@jbonofre
Copy link
Member Author

Thanks for the contribution @jbonofre

I'm still curious to get to know more details about why you want to scan in hidden directories? Can you provide more info about that.

@ottlinger at ASF, we should have license header in all files included in project release source distributions (which is the minimum a project has to provide for each release). Some projects include hidden directories/iles in their source distributions (for instance, Apache Iceberg includes hidden directories like .palantir, .baseline, etc). Those directories/files can't be excluded else it means that it would not be possible to build the project from the source distribution.
To be "clean", these directories/files have to include ASF/license headers and so they should be considered by RAT.

As workaround, RAT gradle|maven plugins use a "custom" walker to scan hidden directories/files. So I think it makes sense to improve our default walker to support this behavior and so having the same support in CLI.

That's basically the background :)

@jbonofre
Copy link
Member Author

I did rebase and added an entry in the changes.
As suggested by @ottlinger I'm updating the tests to use assertj.

@jbonofre
Copy link
Member Author

I started to use assertj in ReportConfigurationTest.

I'm fixing the build.

@ottlinger do you want me to use assertj everywhere ? Maybe it makes more sense to have another PR dedicated to that. Thought ?

@jbonofre
Copy link
Member Author

I pushed the update to define assertj in dependenciesManagement and define where it's required.

@ottlinger please let me know if you want me to refactore all tests using assertj in this PR or in separated one. Thanks !

@ottlinger
Copy link
Contributor

@jbonofre go ahead to use assertj more often in a different PR. Thanks for your contribution.

@jbonofre
Copy link
Member Author

@ottlinger I propose to merge this PR (as it introduces assertj-core dependency). I will create another PR to use assertj everywhere.

@ottlinger ottlinger merged commit ca20f97 into apache:master Nov 29, 2023
16 checks passed
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