moveit2
moveit2 copied to clipboard
ros-planning
Clean up (new PR)
This PR is a continued effort following the PR#140 to address the issue#102. The review comments are going to be addressed here. Therefore, PR#140 can be closed.
A major change here is to refactor the logger names as <package>.<library>.<file> mode. In some situations, <library> is ignored if <library> and <file> are very similar or <library> makes the logger names too heavy. Refer to MIGRATION_GUIDE and discussions here
Actually, I would argue, that including the file into the logger name is not necessary at all. If you want to know the precise location, you can configure your logger output to exactly list the file and line number. Of course, that's not the default. @henningkayser, what do you think?
If we drop logger name from the pattern, would it still be possible to filter out a specific file's logger output?
If we drop the file name from the logger name, would it still be possible to filter a specific file's logger output?
Obviously not. But is this really necessary/desired? Usually, I filter for more higher-level semantic groups. But, I'm open for discussion.
I think there is no perfect solution for all files. In general, I would opt for having a longer and more explicit logger name including the file name while removing all redundancy (i.e. remove prefixes moveit_*...). However, there are several packages where this results in very long names, so we can reduce it even more as long as the logger name is unique. I find it very useful for filtering or grepping log names for a specific file.
The redundant moveit_ prefixes have been removed and a top level scope moveit is used. Now, the message format would be moveit.<package>.<library>.<file>. I think including a file name would possibly reduce extra effort on re-configuring the logger output in some cases.
We are getting closer. I have some more suggestions. In general, the logger scope should be split at semantic entities.
I think these suggestions will not break the scope meaning or the uniqueness of the logger name, so it is OK for me.
It seems to me the logger names are closer to the pattern moveit.<package>.<library>.<file>, but with some flexibility by removing prefixes moveit_, replacing _ with . if not changing the scope meaning and removing the string component which is very similar to their parent scope.
![mergify[bot] avatar](https://avatars.githubusercontent.com/in/10562?v=4 "Published by a pub.dev verified publisher"){kind=small}
@RoboticsYY any updates to this PR?
@tylerjw @henningkayser Should this be rebased and merged or is this obsolete?
@Abishalini I think some more packages have been ported to ROS2, compared to the time when this PR was created. This PR needs a refresh to cover these newly ported packages.
@christhrasher, here is an old PR that is a nice cleanup of a bunch of the names in moveit. Would you be interested in taking over this effort either with a rebase or a new PR that accomplishes the same cleanup?
Created two issues to track finishing this work:
- https://github.com/ros-planning/moveit2/issues/1724
- https://github.com/ros-planning/moveit2/issues/1723