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

[jsk_topic_tools] add JSK_NODELET_LOG_THROTTLE #1446

Merged
merged 2 commits into from Aug 26, 2016

Conversation

Projects
None yet
3 participants
@furushchev
Copy link
Contributor

commented Aug 26, 2016

No description provided.

@furushchev furushchev force-pushed the furushchev:nodelet-throttle branch from f822cf1 to ca57102 Aug 26, 2016

add_executable(test_nodelet_log EXCLUDE_FROM_ALL test/test_nodelet_log.cpp)
target_link_libraries(test_nodelet_log ${catkin_LIBRARIES} ${GTEST_LIBRARIES})
add_dependencies(tests test_nodelet_log)
add_rostest(test/test_nodelet_log.test)

This comment has been minimized.

Copy link
@wkentaro

wkentaro Aug 26, 2016

Member

Is add_rostest_gtest() not usable?

This comment has been minimized.

Copy link
@furushchev

furushchev Aug 26, 2016

Author Contributor

@wkentaro I did not know just how to test by add_rostest_gtest.

This comment has been minimized.

@wkentaro

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

Actually, I found the JSK_ROS_XXX logging macros are not necessary just recently.
Its feature is already covered by ROSCONSOLE_FORMAT environmental variable.
http://wiki.ros.org/rosconsole#Console_Output_Formatting
(you can try it by running export ROSCONSOLE_FORMAT='[${node}] [${function}] [${severity}] [${time}]: ${message}')

I agree that we add this function to jsk_topic_tools at this time, but we should add deprecation warning for all of the logging functions.
like below

Deprecation warning: JSK_ROS_XXX is being deprecated, please consider customize rosconsole output by using ROSCONSOLE_FORMAT environmental variable. See http://wiki.ros.org/rosconsole#Console_Output_Formatting for more details.
@k-okada

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

I agree that we add this function to jsk_topic_tools at this time, but we should add deprecation warning for all of the logging functions.

why we have to add this function at this time, for hydro?

@wkentaro

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

@k-okada It is not necessary. But currently there are already logging macros: JSK_ROS_XXX, JSK_ROS_XXX_STREAM, JSK_ROS_XXX_THROTTLE, JSK_ROS_XXX_STREAM_THROTTLE, JSK_NODELET_XXX, and JSK_NODELET_XXX_STREAM, so I think some users expect JSK_NODELET_XXX_THROTTLE and JSK_NODELET_XXX_STREAM_THROTTLE are also supported, and it is better that we add deprecation warning for them at this time and will remove all macros at once in the next major release. What do you think?

@k-okada

This comment has been minimized.

Copy link
Member

commented Aug 26, 2016

I see, Since we already noticed that JSK_XXX logging macro is no longer needed, so we can create PR that display deprecation warning for these macros

@k-okada k-okada merged commit 69f82da into jsk-ros-pkg:master Aug 26, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@furushchev furushchev deleted the furushchev:nodelet-throttle branch Aug 27, 2016

k-okada added a commit that referenced this pull request Aug 30, 2016

Merge pull request #1447 from wkentaro/fix-1446
[jsk_topic_tools] Some fixes following #1446
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.