search

ros / console_bridge

A ROS-independent package for logging that seamlessly pipes into rosconsole/rosout for ROS-dependent packages.
BSD 3-Clause "New" or "Revised" License
22 stars 62 forks source link

Added linters: cppcheck and cpplint #73

Closed ahcorde closed 4 years ago

ahcorde commented 4 years ago

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 ahcorde@gmail.com

scpeters commented 4 years ago

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

do we need to switch to GitHub actions?

ahcorde commented 4 years ago

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 commented 4 years ago

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

brawner commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

scpeters commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

@scpeters, :construction_worker: travis-ci is working fine :smiley:

ahcorde commented 4 years ago

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