nobleo / rviz_satellite

Display internet satellite imagery in RViz
Apache License 2.0
525 stars 226 forks source link

Code Cleanup #79

Closed beetleskin closed 4 years ago

beetleskin commented 4 years ago

Fixes #76 and #75

Test Plan:

schra commented 4 years ago

I'll look at this once we have merged your other PRs :)

schra commented 4 years ago

Just as a reminder from #72, could you maybe add this small change in this PR too?

When clearing the topic input, the error will be "No map received yet" - maybe we should change that to "No NavSatFix message received yet"?

beetleskin commented 4 years ago

lets do that after review/concerns fixed - just remind me ;)

beetleskin commented 4 years ago

77 is also part of this PR, I'll rebase after #77 is merged

beetleskin commented 4 years ago

@schra are we using a fork of https://github.com/davetcoleman/roscpp_code_format ? what about clang-format and camelCase vs. under_score? Can one set that in the config? Or is that only something clang-tidy can do? Should we add a clang-tidy config? Is there one for ROS? I only know the one from moveit (https://github.com/ros-planning/moveit/blob/master/.clang-tidy). So many questions? :)

schra commented 4 years ago

@schra are we using a fork of https://github.com/davetcoleman/roscpp_code_format

yes

what about clang-format and camelCase vs. under_score? Can one set that in the config? Or is that only something clang-tidy can do? Should we add a clang-tidy config? Is there one for ROS? I only know the one from moveit (https://github.com/ros-planning/moveit/blob/master/.clang-tidy).

That's a really good point. I'll add a remark about this in #78 And yeah, I think you can only do that with clang-tidy :)

beetleskin commented 4 years ago

You missed two variables ;)

inferior humans ..

I rebased, and fixed concerns - the new commit should be merged into the previous one; but I'd keep the others as they are

schra commented 4 years ago

LGTM now, thanks again :)