Skip to content

[ament_uncrustify] Fix file exclusion behavior#334

Merged
audrow merged 3 commits intoament:masterfrom
aprotyas:aprotyas/ament_uncrustify_fix_file_exclusion
Dec 1, 2021
Merged

[ament_uncrustify] Fix file exclusion behavior#334
audrow merged 3 commits intoament:masterfrom
aprotyas:aprotyas/ament_uncrustify_fix_file_exclusion

Conversation

@aprotyas
Copy link
Copy Markdown
Contributor

@aprotyas aprotyas commented Oct 25, 2021

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

audrow
audrow requested changes Oct 27, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

This looks good to me so far. Would you mind adding some tests?

@aprotyas
Copy link
Copy Markdown
Contributor Author

aprotyas commented Oct 27, 2021

Will do! I'll take it out of draft once that's done. Thanks for the initial review.

@aprotyas aprotyas mentioned this pull request Nov 4, 2021
Merged
@aprotyas aprotyas force-pushed the aprotyas/ament_uncrustify_fix_file_exclusion branch from 711bb41 to 7da4a7a Compare November 30, 2021 08:16
@aprotyas
[ament_uncrustify] Fix file exclusion behavior
b57874a 
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 aprotyas force-pushed the aprotyas/ament_uncrustify_fix_file_exclusion branch from 7da4a7a to b57874a Compare November 30, 2021 08:18
@aprotyas
[ament_uncrustify] Add file exclusion tests
cfd6ed3 
Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
@aprotyas aprotyas marked this pull request as ready for review November 30, 2021 08:41
@aprotyas aprotyas requested a review from audrow November 30, 2021 08:42
@aprotyas
Copy link
Copy Markdown
Contributor Author

aprotyas commented Nov 30, 2021 •
edited
Loading

@audrow I've added test cases for the desired file exclusion behavior and rebased on the master branch. There is now parity in the search behavior seen between (at least) ament_copyright, ament_cpplint, and ament_uncrustify.

@aprotyas
[ament_uncrustify] Remove erroneous pytest marker
e7a65ca 
Signed-off-by: Abrar Rahman Protyasha <aprotyas@u.rochester.edu>
@aprotyas
Copy link
Copy Markdown
Contributor Author

aprotyas commented Nov 30, 2021 •
edited
Loading

CI:
Repos file: https://gist.githubusercontent.com/aprotyas/59db75195bf5b8cc23562e9f5658c411/raw/454ae4e42409d38881fe8c9bb9209b9e4001d885/ros2.repos
Build args: --packages-above-and-dependencies ament_uncrustify
Test args: --packages-above ament_uncrustify
Job: https://ci.ros2.org/job/ci_launcher/9407

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

audrow
audrow approved these changes Nov 30, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@audrow audrow left a comment

Choose a reason for hiding this comment

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

Looks good with green CI.

@aprotyas
Copy link
Copy Markdown
Contributor Author

aprotyas commented Nov 30, 2021

@audrow do you know what the test failure is about? I remember coming across this on another PR in this repository but not sure if it's related.

@audrow
Copy link
Copy Markdown
Contributor

audrow commented Dec 1, 2021

I think that error should be fixed now. Here's another CI run:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@aprotyas
Copy link
Copy Markdown
Contributor Author

aprotyas commented Dec 1, 2021

Running Windows CI again since there was a network problem:

  • Windows Build Status

@audrow audrow merged commit 0a7c40f into ament:master Dec 1, 2021
@audrow
Copy link
Copy Markdown
Contributor

audrow commented Dec 1, 2021

Thanks for the PR, @aprotyas!

@aprotyas aprotyas deleted the aprotyas/ament_uncrustify_fix_file_exclusion branch December 1, 2021 18:25
@clalancette clalancette mentioned this pull request Dec 16, 2021
Closed
@aprotyas aprotyas mentioned this pull request Dec 26, 2021
Closed
@aprotyas aprotyas mentioned this pull request Jan 30, 2022
Open
orensbruli pushed a commit to orensbruli/ament_lint that referenced this pull request Sep 9, 2022
@aprotyas @orensbruli
[ament_uncrustify] Fix file exclusion behavior (ament#334)
7a48e3a 
* [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

Reviewers

@audrow audrow audrow approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@aprotyas @audrow