Fix linter error in c++ code (please consider sending PR with linter test)

[wkentaro]

IDEA I'd like to automate the style check by maintainers. Using roslint http://wiki.ros.org/roslint This is not so important in short term but I think it will be meaningful in long term.

SOLUTION for python code, we can use autopep8 https://pypi.python.org/pypi/autopep8/ to format existing code.

But for c++ there is no such tools. ( I couldn't find it.) so maybe it needs to be fixed manually.

CONCLUTION So please consider to format code when sending PR with roslint_cpp test like: https://github.com/jsk-ros-pkg/jsk_common/blob/master/jsk_topic_tools/CMakeLists.txt#L101-L104 example PR is here: https://github.com/jsk-ros-pkg/jsk_recognition/pull/1417

[garaemon]

Need to think about pros and cos before being a fashionista.

• Pros
  • Guaranteed consistency
  • Auto-format
• Cons
  • Coding style itself is not important.
  • Slow development.
  • Too many rules to develop.
  • Education cost of linter tool.
  • The style used in roslint does not match current coding style. It is important to follow the style of existing code rather than roslint. (roslint itself is just a newbie.)

[wkentaro]

Please stop looking down on me by saying me as a fashionista. I'm considering style check is important after using pep8, flake8 for python and cpplint for cpp for a while.

• https://www.python.org/dev/peps/pep-0008/
• http://google.github.io/styleguide/cppguide.html

Pros

• Guaranteed consistency, (-> so easy to read.)
• Auto-format, (-> so save time to review.)

Cons

• Coding style itself is not important. (-> Is this cons?)
• Too many rules to develop, so education cost become high and causes slow development. (-> Of course we should create only rules that's really important. And I need to share catkin build --this --no-deps --catkin-make-args roslint as well as catkin build --this --no-deps --catkin-make-args run_tests)
• The style used in roslint does not match current coding style. It is important to follow the style of existing code rather than roslint. (-> we can ignore some errors with --filter option)

Also, by creating our own linter for jsk_recognition, we can automate the syntax check when creating new nodes. (like forgotten calling onInitPostProcess() for jsk_topic_tools::ConnectionBasedNodelet and vital->poke() in callback for jsk_topic_tools::DiagnosticNodelet, something like that.)

[k-okada]

no one is looking down on others, Assume good faith See http://wiki.ros.org/Support#Etiquette

My thought on this is; we should make this kind of effort on upstream/stable packages, to maximize it's benefit, Apply roslint is good, may be better to run within travis, but not marked as fail for a while.

◉ Kei Okada

On Thu, Dec 17, 2015 at 12:47 AM, Kentaro Wada notifications@github.com wrote:

Please stop looking down on me by saying me as a fashionista. I'm considering style check is important after using pep8, flake8 for python and cpplint for cpp for a while.

• https://www.python.org/dev/peps/pep-0008/
• http://google.github.io/styleguide/cppguide.html

Pros

• Guaranteed consistency, (-> so easy to read.)
• Auto-format, (-> so save time to review.)

Cons

• Coding style itself is not important. (-> Is this cons?)
• Too many rules to develop, so education cost become high and causes slow development. (-> Of course we should create only rules that's really important. And I need to share catkin build --this --no-deps --catkin-make-args roslint as well as catkin build --this --no-deps --catkin-make-args run_tests)
• The style used in roslint does not match current coding style. It is important to follow the style of existing code rather than roslint. (-> we can ignore some errors with --filter option)

Also, by creating our own linter for jsk_recognition, we can automate the syntax check when creating new nodes. (like forgotten calling onInitPostProcess() for jsk_topic_tools::ConnectionBasedNodelet and vital->poke() in callback for jsk_topic_tools::DiagnosticNodelet, something like that.)

— Reply to this email directly or view it on GitHub https://github.com/jsk-ros-pkg/jsk_recognition/issues/1416#issuecomment-165151190 .

[k-okada]

for example https://github.com/ros-perception/vision_opencv/pull/56 is upstream/stable packges for me, and if someone proposed that we have to follow roslit for this package, everyone will agree. So if we are able to move some of nodes to the pcl_ros package, then it ok. The problem is how to choose node which should be moved from jsk_pcl_ros to pcl_ros, in the opencv_apps, we choose sample programs in https://github.com/Itseez/opencv/tree/master/samples/cpp, if we could not find rules, it's very difficult to choose and this becomes endress game.

◉ Kei Okada

On Thu, Dec 17, 2015 at 11:26 AM, Kei Okada k-okada@jsk.t.u-tokyo.ac.jp wrote:

no one is looking down on others, Assume good faith See http://wiki.ros.org/Support#Etiquette

My thought on this is; we should make this kind of effort on upstream/stable packages, to maximize it's benefit, Apply roslint is good, may be better to run within travis, but not marked as fail for a while.

◉ Kei Okada

On Thu, Dec 17, 2015 at 12:47 AM, Kentaro Wada notifications@github.com wrote:

Please stop looking down on me by saying me as a fashionista. I'm considering style check is important after using pep8, flake8 for python and cpplint for cpp for a while.

• https://www.python.org/dev/peps/pep-0008/
• http://google.github.io/styleguide/cppguide.html

Pros

• Guaranteed consistency, (-> so easy to read.)
• Auto-format, (-> so save time to review.)

Cons

• Coding style itself is not important. (-> Is this cons?)
• Too many rules to develop, so education cost become high and causes slow development. (-> Of course we should create only rules that's really important. And I need to share catkin build --this --no-deps --catkin-make-args roslint as well as catkin build --this --no-deps --catkin-make-args run_tests)
• The style used in roslint does not match current coding style. It is important to follow the style of existing code rather than roslint. (-> we can ignore some errors with --filter option)

Also, by creating our own linter for jsk_recognition, we can automate the syntax check when creating new nodes. (like forgotten calling onInitPostProcess() for jsk_topic_tools::ConnectionBasedNodelet and vital->poke() in callback for jsk_topic_tools::DiagnosticNodelet, something like that.)

— Reply to this email directly or view it on GitHub https://github.com/jsk-ros-pkg/jsk_recognition/issues/1416#issuecomment-165151190 .

[wkentaro]

no one is looking down on others, Assume good faith See http://wiki.ros.org/Support#Etiquette

I see, that's the rule in ROS community.

My thought on this is; we should make this kind of effort on upstream/stable packages, to maximize it's benefit, Apply roslint is good, may be better to run within travis, but not marked as fail for a while.

That's an idea, I agree with you. IMO, roslint should not be the limitation of fast developing, but help of that, and I think it can be by using it correctly. (even if for jsk_recognition package) It can detect

• missing include guard
• hard tab

commit 8fac41a54c6fa0bcd025f6ebaeaa6f53b2410c0b
Author: Ryohei Ueda <garaemon@gmail.com>
Date:   Wed Aug 26 08:03:03 2015 +0900

    [jsk_pcl_ros/ColorizeSegmentedRF] Fix include guard not to collide with colorize_random_points_rf.h

commit b4dde6f9340dc0ece4e921e0d8df5afd13f257b7
Author: Ryohei Ueda <garaemon@gmail.com>
Date:   Wed Aug 26 07:57:46 2015 +0900

    [jsk_pcl_ros/MaskImageToDepthConsideredMaskImage] Fix include guard
commit 32d949f36d03171a12172192f18f35162f414f5f
Author: Ryohei Ueda <garaemon@gmail.com>
Date:   Fri Nov 28 02:22:07 2014 +0900

    Fix coding style of DepthImageCreator:
    * remove hard tabs
    * add bsd header