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

Linter fixes #2879

Merged
merged 16 commits into from
Apr 4, 2022
Merged

Linter fixes #2879

merged 16 commits into from
Apr 4, 2022

Conversation

padhupradheep
Copy link
Member

Some more linter fixes..

@mergify
Copy link
Contributor

mergify bot commented Mar 31, 2022

@padhupradheep, please properly fill in PR template in the future. @SteveMacenski, use this instead.

  • Check that any new parameters added are updated in navigation.ros.org
  • Check that any significant change is added to the migration guide
  • Check that any new features OR changes to existing behaviors are reflected in the tuning guide
  • Check that any new functions have Doxygen added
  • Check that any new features have test coverage
  • Check that any new plugins is added to the plugins page
  • If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

@padhupradheep
Copy link
Member Author

padhupradheep commented Mar 31, 2022 •
edited
Loading

Ok with the last commit, now we only have 6 more linting errors. It is all in nav2_amcl to make it easy.. I was not sure, how to fix those.

@mergify
Copy link
Contributor

mergify bot commented Mar 31, 2022

@padhupradheep, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

@padhupradheep
Copy link
Member Author

padhupradheep commented Mar 31, 2022 •
edited
Loading

You will see CI complaining about uncrustify in inflation_layer.cpp, if I try to satisfy them, then I get
If an else has a brace on one side, it should have it on both [readability/braces] [5] from cpplint.

I will check that tomorrow.

SteveMacenski
SteveMacenski requested changes Mar 31, 2022
View reviewed changes
nav2_amcl/src/include/portable_utils.h Outdated Show resolved Hide resolved
nav2_costmap_2d/include/nav2_costmap_2d/voxel_layer.hpp Outdated Show resolved Hide resolved
nav2_costmap_2d/test/unit/keepout_filter_test.cpp
static const std::string FILTER_NAME = "keepout_filter";
static const std::string INFO_TOPIC = "costmap_filter_info";
static const std::string MASK_TOPIC = "mask";
static const char FILTER_NAME[]{"keepout_filter"};
Copy link
Member

Choose a reason for hiding this comment

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

Why not leave as string, you just cast to string later

Copy link
Member Author

@padhupradheep padhupradheep Apr 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I wanted too, but the linters complained..

For a static/global string constant, use a C style string instead: "static const char FILTER_NAME[]

Copy link
Member

Choose a reason for hiding this comment

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

you could remove const then

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm..

Static/global string variables are not permitted.

Let me check for alternates

nav2_costmap_2d/test/unit/speed_filter_test.cpp
static const std::string INFO_TOPIC = "costmap_filter_info";
static const std::string MASK_TOPIC = "mask";
static const std::string SPEED_LIMIT_TOPIC = "speed_limit";
static const char FILTER_NAME[]{"speed_filter"};
Copy link
Member

Choose a reason for hiding this comment

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

Ditto

nav2_costmap_2d/plugins/inflation_layer.cpp Outdated Show resolved Hide resolved
@SteveMacenski
Copy link
Member

Still got


--- plugins/inflation_layer.cpp
+++ plugins/inflation_layer.cpp.uncrustify
@@ -445 +445,2 @@
-        cost_scaling_factor_ != parameter.as_double()) {
+        cost_scaling_factor_ != parameter.as_double())
+      {
@@ -456 +457,2 @@
-        inflate_unknown_ != parameter.as_bool()) {
+        inflate_unknown_ != parameter.as_bool())
+      {
@@ -460 +462,2 @@
-        inflate_around_unknown_ != parameter.as_bool()) {
+        inflate_around_unknown_ != parameter.as_bool())
+      {

in CI

@padhupradheep
Copy link
Member Author

Yup that’s what I had mentioned in one of the previous comments. If I satisfy uncrustify, lint complain and vice versa.. 😅 I’ll take a look at it tonight

@padhupradheep
Copy link
Member Author

ament/ament_lint#158

I was getting OCD on repeating the loop! Praise the lord, I broke the loop by finding this issue :-D

@SteveMacenski SteveMacenski merged commit a5a3735 into ros-navigation:main Apr 4, 2022
redvinaa pushed a commit to EnjoyRobotics/navigation2 that referenced this pull request Jun 30, 2022
@padhupradheep @redvinaa
Linter fixes (ros-navigation#2879)
688d25c 
* satisfying uncrustify

* cpplint fixes

* linter fixes

* some more fixes

* fix build - go back

* build and linter both satisfied

* crustify partial fix

* reverting back fix for datatype in amcl

* more amcl cleaning

* update

* no linters

* copyright fixes

* moving portable_utils
@padhupradheep padhupradheep deleted the linter-fixes branch July 22, 2022 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

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