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_cmake_flake8 ignores value of AMENT_LINT_AUTO_FILE_EXCLUDE #423

Open
RFRIEDM-Trimble opened this issue Nov 16, 2022 · 5 comments
Open
Labels
help wanted Extra attention is needed

Comments

@RFRIEDM-Trimble
Copy link
Contributor

RFRIEDM-Trimble commented Nov 16, 2022

When I use ament_lint_auto and want to exclude certain files, only a subset of the linters obey that variable AMENT_LINT_AUTO_FILE_EXCLUDE.

This issue is specific to ament_cmake_flake8. If you take a look at #386, you can see exclude logic was never added to it.

@RFRIEDM-Trimble RFRIEDM-Trimble changed the title ament_cmake_flake8 ignored value of AMENT_LINT_AUTO_FILE_EXCLUDE ament_cmake_flake8 ignores value of AMENT_LINT_AUTO_FILE_EXCLUDE Nov 16, 2022
@aprotyas
Copy link
Contributor

@RFRIEDM-Trimble when I was writing #386 I only updated the CMake hooks that actually had file exclusion provisions in place. It looks like ament_cmake_flake8 just doesn't support file exclusion yet - i.e. there's no EXCLUDE argument to the CMake function ament_flake8.

Do you mind adding that support? Check out some of the other packages for how it's done, it should be relatively trivial and I can review the PR you submit for it.

General exclusion can be achieved like so:

if(ARG_EXCLUDE)
list(APPEND cmd "--exclude" "${ARG_EXCLUDE}")
endif()

Exclusion through the AMENT_LINT_AUTO_FILE_EXCLUDE interface can be achieved like so:

set(_all_exclude "")
if(DEFINED ament_cmake_cpplint_ADDITIONAL_EXCLUDE)
list(APPEND _all_exclude ${ament_cmake_cpplint_ADDITIONAL_EXCLUDE})
endif()
if(DEFINED AMENT_LINT_AUTO_FILE_EXCLUDE)
list(APPEND _all_exclude ${AMENT_LINT_AUTO_FILE_EXCLUDE})
endif()
message(STATUS "Configured cpplint exclude dirs and/or files: ${_all_exclude}")
ament_cpplint(EXCLUDE ${_all_exclude})

@RFRIEDM-Trimble
Copy link
Contributor Author

@RFRIEDM-Trimble when I was writing #386 I only updated the CMake hooks that actually had file exclusion provisions in place. It looks like ament_cmake_flake8 just doesn't support file exclusion yet - i.e. there's no EXCLUDE argument to the CMake function ament_flake8.

Do you mind adding that support? Check out some of the other packages for how it's done, it should be relatively trivial and I can review the PR you submit for it.

General exclusion can be achieved like so:

if(ARG_EXCLUDE)
list(APPEND cmd "--exclude" "${ARG_EXCLUDE}")
endif()

Exclusion through the AMENT_LINT_AUTO_FILE_EXCLUDE interface can be achieved like so:

set(_all_exclude "")
if(DEFINED ament_cmake_cpplint_ADDITIONAL_EXCLUDE)
list(APPEND _all_exclude ${ament_cmake_cpplint_ADDITIONAL_EXCLUDE})
endif()
if(DEFINED AMENT_LINT_AUTO_FILE_EXCLUDE)
list(APPEND _all_exclude ${AMENT_LINT_AUTO_FILE_EXCLUDE})
endif()
message(STATUS "Configured cpplint exclude dirs and/or files: ${_all_exclude}")
ament_cpplint(EXCLUDE ${_all_exclude})

Awesome, good to know I wasn't missing anything major; seems pretty minor. Yep, I'm happy to contribute a change. Do you think it's a good idea to clarify for the ament_lint_auto package as part of contributing, all linters should support the argument, for any future additions?

Feel free to assign this issue to me, I'll get a fix in.

@clalancette clalancette added the help wanted Extra attention is needed label Dec 1, 2022
@Ryanf55
Copy link

Ryanf55 commented Jan 18, 2023

This can be closer since the PR is merged. Well, since it's a bugfix, can we backport it to humble? On humble, the docs disagree with the linter behavior which is why one would assume it would work.

@Benjamin-Tan
Copy link

Any chance for this PR (#424) to be backport to humble?

@Ryanf55
Copy link

Ryanf55 commented Nov 17, 2023

Any chance for this PR (#424) to be backport to humble?

Please consider backporting. It was my intention doing the original work that it would be backported. I've been moving to black so I need to start being able to turn off flake8 on files, and do it humble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants