Conversation
|
yes. I have already tried to split them into the following to submit:
Actually, I really know your consideration for that "the smaller the better to review", but because the different files have more or less the similar/relevant changes and some of them are dependent each other, we can consider merge them together, and if I file a PR for a file changing, it doesn't make sense, I think. thanks ! |
|
In addition, it works for a while before moving here, of course, if you have some good suggestion to operate this, that's really welcomed 😄 |
- modify package.xml with ros2 dependencies and ament tools - replace CMakeLists.txt with ROS2 ament rules - add missing message_event.h message_trait.h parameter_adapter.h - remove Boost codes with standard C+11/C++14 - use rclcpp APIs for the previous roscpp - use ros2 time interfaces
|
@gaoethan Please follow the suggested splitting described in #2 as @mikaelarguedas mentioned above. Otherwise it unnecessarily complicates the review as well as delay the patch actually getting merged. I understand that this requires a little bit more effort but that is the established approach for reviewing and accepting larger contributions. Thanks. |
- remove boost dependency - use ros2 rclcpp and time interface
|
Close this PR and try to submit the other small PRs later, thanks ! |
|
This is a high priority for us to get merged as it is needed to support image_transport being ported. @gaoethan are you actively working on this? |
|
@tfoote yes, actually I'm working on it, but it's been a little busy with something else recently, I initially deem those depending on this can use and test with the current one and it's just needed to keep the pace to move it here. If you have the priority and I'll try to submit new small patches ASAP, thanks ! |
|
@gaoethan I've started breaking down the PR into separate commits and refactoring in this branch: https://github.com/ros2/message_filters/tree/refactor4 There's still a bunch of comments TODOs etc from my reviewing. I have also cleaned up several things that I flagged. I'm headed out for the night now, but will be back working on this tomorrow. |
|
ok. thanks for your bandwidth and help to move it here, so...in order to not make it messy and keep the coherence, I'll temporarily stop my patches, and then go ahead for further development or issues if it's necessary while you finish you initial refactoring on a whole. thanks @tfoote |
It mainly includes C++ code in directory of
includeandsrc