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

Added linters: cppcheck and cpplint #73

Merged
merged 10 commits into from
May 21, 2020
Merged

Added linters: cppcheck and cpplint #73

merged 10 commits into from
May 21, 2020

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented May 13, 2020

Added cppcheck and cpplint to reach QL2.

The strategy it's to download the cppcheck and cpplint scripts from ament_lint package and run them as tests.

@brawner @Blast545 what do you think?

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde self-assigned this May 13, 2020
Copy link
Contributor

@Blast545 Blast545 left a comment

Choose a reason for hiding this comment

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

Why not adding ament_copyright as well? To deal with the license/copyright parts of the REP-2004.

Otherwise, sounds as a cool approach, those tests will appear under CI for console_bridge_vendor?

test/CMakeLists.txt Outdated Show resolved Hide resolved
test/CMakeLists.txt Show resolved Hide resolved
test/CMakeLists.txt Show resolved Hide resolved
@Blast545 Blast545 mentioned this pull request May 13, 2020
@scpeters
Copy link
Contributor

it looks like travis-ci didn't run on the latest commit

do we need to switch to GitHub actions?

Signed-off-by: ahcorde <[email protected]>
@ahcorde
Copy link
Contributor Author

ahcorde commented May 13, 2020

I added an empty commit to re-run CI. Right now travis-ci is working but failing. ( I think it's because of the python version)

@scpeters
Copy link
Contributor

it looks like travis is using trusty for CI; feel free to update that to a more recent version

@brawner
Copy link
Contributor

brawner commented May 13, 2020

This is a really cool PR. Since this is neither a ROS 1 package nor a ROS 2 package (technically), what's the rationale for using ament's linters and configurations? Are there other more appropriate lint configurations for this package's 'style'?

@ahcorde
Copy link
Contributor Author

ahcorde commented May 13, 2020

Ament's linters wrap some utilities such as cppcheck or cpplint. I'm downloading these scripts from our packages to use the same version as in ROS 2.

For example cppcheck is integrated in CMake, adding the following line:

set(CMAKE_CXX_CPPCHECK "cppcheck")

but the linter will be checked during the compilation process and we want to include this as a specific tests that's why I added the logic.

We can add uncrustify.

@brawner
Copy link
Contributor

brawner commented May 13, 2020

I guess the question is: why ROS 2 style configurations instead of ROS 1 style?

@scpeters
Copy link
Contributor

I guess the question is: why ROS 2 style configurations instead of ROS 1 style?

I think it could go either way. ROS 2 style sounds fine to me

@brawner
Copy link
Contributor

brawner commented May 13, 2020

Totally. I don't have a preference one way or the other, but I was worried others might. I just wanted to capture some of the thought behind the choice. This PR cuts down line lengths to 100 from 120 for example.

@scpeters
Copy link
Contributor

Totally. I don't have a preference one way or the other, but I was worried others might.

we can listen for objections, but in the absence of any, I think the ros2 style can be used

@ahcorde
Copy link
Contributor Author

ahcorde commented May 13, 2020

My thought was the following: this request came from ROS 2 Quality levels. I can't remember any ROS 1 packages taking care about linters. That's why I decided use ament's linters. Open to include/remove others

@ahcorde
Copy link
Contributor Author

ahcorde commented May 14, 2020

@scpeters, 👷 travis-ci is working fine 😃

Signed-off-by: ahcorde <[email protected]>
test/CMakeLists.txt Outdated Show resolved Hide resolved
@ahcorde
Copy link
Contributor Author

ahcorde commented May 15, 2020

@scpeters I think all the comments have been addressed. Feel free to merge it when you consider

find_package(PythonInterp 3 REQUIRED)

if(NOT PYTHONINTERP_FOUND)
message(STATUS "No PythonInterp found. Linters will not be executed")
Copy link
Contributor

Choose a reason for hiding this comment

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

I think at the STATUS level, these messages will not be shown during a colcon build, though they would be at a WARNING level. I think that is fine, but I wanted to confirm this is the style of verbosity that is preferred

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed c8efe01

Signed-off-by: ahcorde <[email protected]>
@ahcorde ahcorde requested a review from scpeters May 21, 2020 09:11
@scpeters scpeters merged commit fd6f82c into master May 21, 2020
@scpeters scpeters deleted the ahcorde/linters branch May 21, 2020 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants