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.

Sign up for GitHub

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

Jump to bottom

[ament_uncrustify] Fix file exclusion behavior #334

Merged
merged 3 commits into from
Dec 1, 2021
Merged

[ament_uncrustify] Fix file exclusion behavior #334

merged 3 commits into from
Dec 1, 2021

Conversation

aprotyas
Copy link
Contributor

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
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
Contributor Author

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
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
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
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
Contributor Author

@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
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
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
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