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

[ament_copyright, ament_uncrustify] Unexpected file exclusion behavior #326

Closed
aprotyas opened this issue Oct 17, 2021 · 1 comment
Closed

Comments

@aprotyas
Copy link
Contributor

aprotyas commented Oct 17, 2021

Problem description

The --exclude flag for ament_copyright/ament_uncrustify does not work as expected, or at least not how file exclusion works in some other packages like ament_cpplint and ament_cppcheck.

The issues I've noted:

  • File paths (both relative and absolute) don't work for the arguments to the --exclude flag. Only the excluded filename(s) can be provided.
  • As a consequence of the above issue, provided filenames are excluded indiscriminately throughout its search tree.

In any case, this behavior is not consistent with how exclusion works (and/or should work in my opinion) for some other linter packages.

Problem demonstration

For the following examples, let the following search tree. The expected behavior is in bold. Output is shown for ament_copyright but the same idea applies for ament_uncrustify.

.
├── dir
│  └── src.cpp
└── src.cpp

Example 1: File paths don't work

ament_copyright . --exclude dir/src.cpp should only exclude dir/src.cpp. Instead, neither of the files are excluded and it outputs:

No problems found, checked 2 files

Example 2: Indiscriminate matching to a filename

ament_copyright . --exclude src.cpp should only exclude src.cpp. Instead, both of the files are excluded and it outputs:

No repository roots and files found
No problems found, checked 0 files

Comments

I reckon this exclusion behavior can be remedied if a solution like #299 is used. On that note, I've opened #327. If maintainers think those changes are good, I will give a similar treatment to ament_uncrustify in a separate PR.

Another thought is that the "correct" code to filter out excluded files can be put in a separate utility module that all the linting packages can access, because it looks like every ament_[linter] is doing it slightly differently...

aprotyas added a commit to aprotyas/ament_lint that referenced this issue Oct 17, 2021
This commit fixes the faulty file exclusion behavior reported in
ament#326.

Specifically, it walks down the path where `ament_copyright` has
been invoked to build a map of files to be tested, respecting
the exclusion list by globbing and matching against files in the
tree.

Changes inspired by ament#299.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
aprotyas added a commit to aprotyas/ament_lint that referenced this issue Oct 17, 2021
This commit fixes the faulty file exclusion behavior reported in
ament#326.

Specifically, the exclusion list is matched against traversed
files in the `crawler` module.

Changes inspired by ament#299.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
@aprotyas aprotyas changed the title [ament_copyright] Unexpected file exclusion behavior [ament_copyright, ament_uncrustify] Unexpected file exclusion behavior Oct 17, 2021
aprotyas added a commit to aprotyas/ament_lint that referenced this issue Oct 25, 2021
This PR fixes the file exclusion behavior reported in ament#326.

Specifically, the exclusion list is matched against
files/directories as the search path is traversed.

Tries to maintain consistency with ament#327.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
audrow pushed a commit that referenced this issue Nov 4, 2021
* [ament_copyright] Fix file exclusion behavior

This commit fixes the faulty file exclusion behavior reported in
#326.

Specifically, the exclusion list is matched against traversed
files in the `crawler` module.

Changes inspired by #299.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* Update excluded file path in copyright tests

Since file names are not indiscriminately matched throughout the
search tree anymore, the excluded files listed in the copyright
tests need to be updated relative to the root of the package.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* Add test cases to check exclusion behavior

Specifically, these tests check for:

- Incorrect exclusion of single filenames.
- Correct exclusion of relatively/absolutely addressed filenames.
- Correct exclusion of wildcarded paths.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* Add unit tests for crawler module

These unit tests make sure both search and exclusion behaviors are
correctly demonstrated by the `ament_copyright.crawler` module.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
aprotyas added a commit to aprotyas/ament_lint that referenced this issue Nov 30, 2021
This PR fixes the file exclusion behavior reported in ament#326.

Specifically, the exclusion list is matched against
files/directories as the search path is traversed.

Tries to maintain consistency with ament#327.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
aprotyas added a commit to aprotyas/ament_lint that referenced this issue Nov 30, 2021
This PR fixes the file exclusion behavior reported in ament#326.

Specifically, the exclusion list is matched against
files/directories as the search path is traversed.

Tries to maintain consistency with ament#327.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
audrow pushed a commit that referenced this issue Dec 1, 2021
* [ament_uncrustify] Fix file exclusion behavior

This PR fixes the file exclusion behavior reported in #326.

Specifically, the exclusion list is matched against
files/directories as the search path is traversed.

Tries to maintain consistency with #327.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* [ament_uncrustify] Add file exclusion tests

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* [ament_uncrustify] Remove erroneous pytest marker

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
@aprotyas
Copy link
Contributor Author

Since #327 and #334 have merged, this issue is resolved.

orensbruli pushed a commit to orensbruli/ament_lint that referenced this issue Sep 9, 2022
* [ament_uncrustify] Fix file exclusion behavior

This PR fixes the file exclusion behavior reported in ament#326.

Specifically, the exclusion list is matched against
files/directories as the search path is traversed.

Tries to maintain consistency with ament#327.

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* [ament_uncrustify] Add file exclusion tests

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>

* [ament_uncrustify] Remove erroneous pytest marker

Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
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

No branches or pull requests

1 participant