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

[ros2] Lint/make utils file #153

Merged
merged 1 commit into from
Jun 8, 2021

Conversation

flynneva
Copy link
Collaborator

  • fix a bunch of lint errors
  • move usb_cam.h to usb_cam.hpp (should we keep usb_cam.h and just include the .hpp version for awhile?)
  • move utlitiy functions out of usb_cam.cpp and into usb_cam_utils.hpp to keep the code clean & separate

@flynneva
Copy link
Collaborator Author

@Karsten1987 wanted to get your opinion here. I cant think of any drawbacks to moving usb_cam.h to .hpp, can you? also, a few of the linting fixes (specifically the copyright ones) I wasnt sure about.

@flynneva flynneva changed the title Lint/make utils file [ros2] Lint/make utils file May 18, 2021
@flynneva
Copy link
Collaborator Author

also a pretty major change in this one is the creation of the usb_cam_utilities.hpp file....just to hold the "helper" methods that were in the usb_cam.hpp header file. I figured these should be separate since there was one that was absolutely huge and just a bunch of numbers.

@Karsten1987
Copy link

The way I'd consider this is that at this moment you don't have to deal with API/ABI or even style/behavior changes on the ROS2 branch as there aren't any binary releases for it yet, is that right?
I'd propose to finish up all major changes regarding functionality in one commit, having all lint/cppcheck related, non functional changes in a separate commit. Creating a utilities header lies somewhere in between, but I believe it doesn't matter much.
As for licensing, please be aware that the Bosch LLC is still a copyright holder of this package - even though we mostly abandoned it :( That being said, I am not sure how I feel about having a LICENSE_OLD file in there. There really should only be one license file present.

@flynneva
Copy link
Collaborator Author

flynneva commented May 18, 2021

hi @Karsten1987, yes sorry about the LICENSE_OLD, ill remove it. The "new" license is the same as the old just with different formatting and the copyright is most definitely still in Robert Bosch, LLC's name if you look at the files (even the new one I made called usb_cam_utils.hpp since it is a direct "copy paste" from the usb_cam.h file.

i'll merge together all these updates into one commit so we can move forward

@flynneva
Copy link
Collaborator Author

flynneva commented Jun 7, 2021

@Karsten1987 pinging you one more time to look this PR over before merging. let me know what you think.

Copy link

@Karsten1987 Karsten1987 left a comment

Choose a reason for hiding this comment

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

lgtm!

@flynneva flynneva merged commit c112345 into ros-drivers:ros2 Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants