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

Alternative versions of ros::param::param and ros::NodeHandle::param with return value #592

Closed
wants to merge 5 commits into
base: indigo-devel
from

Conversation

Projects
None yet
6 participants
@xperroni
Contributor

xperroni commented Mar 31, 2015

(Note: my editor automatically removed a number of line-ending whistespaces in "node_handle.h". If this is a problem, feel free to reject this pull request, and I'll resubmit it with these changes rolled back.)

Template methods ros::param::param() and ros::NodeHandle::param() set a
variable either to the value of a parameter or, if not available, a predefined
default value. Often the variable is passed straight to a constructor or
function call and then disregarded.

The alternative versions of the param() methods introduced here enable
retrieving the result value from a function return, rather than through an
output variable. This makes possible to write more streamlined code.

For example, the code below:

std::string path;
std::string format;
double fps;

param("~path", path, "video.mpg");
param("~format", format, "MPEG");
param("~fps", fps, 30.0);

VideoRecorder recorder(path, format, fps);

Could be more succinctly written as:

VideoRecorder recorder(
    param<std::string>("~path", "video.mpg"),
    param<std::string>("~format", "MPEG"),
    param<double>("~fps", 30.0)
);

Or:

NodeHandle nh("~");

VideoRecorder recorder(
    nh.param<std::string>("path", "video.mpg"),
    nh.param<std::string>("format", "MPEG"),
    nh.param<double>("fps", 30.0)
);

Signed-off-by: Helio Perroni Filho xperroni@gmail.com

Alternative versions of ros::param::param and ros::NodeHandle::param …
…with return value

Template methods ros::param::param() and ros::NodeHandle::param() set a
variable either to the value of a parameter or, if not available, a predefined
default value. Often the variable is passed straight to a constructor or
function call and then disregarded.

The alternative versions of the param() methods introduced here enable
retrieving the result value from a function return, rather than through an
output variable. This makes possible to write more streamlined code.

For example, the code below:

    std::string path;
    std::string format;
    double fps;

    param("~path", path, "video.mpg");
    param("~format", format, "MPEG");
    param("~fps", fps, 30.0);

    VideoRecorder recorder(path, format, fps);

Could be more succinctly written as:

    VideoRecorder recorder(
        param<std::string>("~path", "video.mpg"),
        param<std::string>("~format", "MPEG"),
        param<double>("~fps", 30.0)
    );

Or:

    NodeHandle nh("~");

    VideoRecorder recorder(
        nh.param<std::string>("path", "video.mpg"),
        nh.param<std::string>("format", "MPEG"),
        nh.param<double>("fps", 30.0)
    );

Signed-off-by: Helio Perroni Filho <xperroni@gmail.com>
@ros-pull-request-builder

This comment has been minimized.

Member

ros-pull-request-builder commented Mar 31, 2015

Can one of the admins verify this patch?

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Mar 31, 2015

Thank you for the patch. As you also mentioned it please remove any whitespace changes since they make backporting changes between branches way more difficult.

@xperroni

This comment has been minimized.

Contributor

xperroni commented Mar 31, 2015

OK. Should I close the pull request and submit it again? I couldn't find a
way to edit the request directly, or even restore the deleted whitespace
without first rolling back the original commit.

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Apr 1, 2015

Simply continue committing to the branch on your forked repo and the commits will "appear" in this PR.

Reversed whitespace deletions
Signed-off-by: Helio Perroni Filho <xperroni@gmail.com>
@xperroni

This comment has been minimized.

Contributor

xperroni commented Apr 1, 2015

Done. Sorry it took this long to sort it out.

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Apr 1, 2015

@ros-pull-request-builder

This comment has been minimized.

Member

ros-pull-request-builder commented Apr 1, 2015

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/175/

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Apr 1, 2015

@esteve @tfoote @wjwwood Please review.

@esteve

This comment has been minimized.

Member

esteve commented Apr 1, 2015

+1

@@ -528,6 +528,38 @@ TEST(Params, mapBoolParam)
ASSERT_TRUE(std::equal(map_b.begin(), map_b.end(), map_b2.begin()));
}
TEST(Params, paramTemplateFunction)
{
EXPECT_TRUE( param::param<std::string>( "string", "" ) == "test" );

This comment has been minimized.

@tfoote

tfoote Apr 1, 2015

Member

This and the other EXPECT_TRUE on strings being equal should use EXPECT_EQ docs on string comparisions It gives better visibility into the error reports.

@wjwwood

This comment has been minimized.

Member

wjwwood commented Apr 1, 2015

lgtm, +1

@tfoote

This comment has been minimized.

Member

tfoote commented Apr 1, 2015

Thanks for putting this together.

lgtm with the small fixup.

Changed assertion used to check string results
Signed-off-by: Helio Perroni Filho <xperroni@gmail.com>
@xperroni

This comment has been minimized.

Contributor

xperroni commented Apr 2, 2015

OK, changed the assertion used to check the string results as pointed.

@ros-pull-request-builder

This comment has been minimized.

Member

ros-pull-request-builder commented Apr 2, 2015

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/176/

@tfoote

This comment has been minimized.

Member

tfoote commented Apr 2, 2015

lgtm

EXPECT_TRUE( param::param<std::string>( "string", "" ) == "test" );
EXPECT_TRUE( param::param<std::string>( "gnirts", "test" ) == "test" );
EXPECT_TRUE( param::param<int>( "int", 0 ) == 10 );

This comment has been minimized.

@esteve

esteve Apr 2, 2015

Member

Please apply change suggested by @tfoote here too (EXPECT_EQ instead of EXPECT_TRUE)

This comment has been minimized.

@xperroni

xperroni Apr 3, 2015

Contributor

Argh, forgot about those other two. Changed as requested.

Changed assertion used to check string results (2)
Signed-off-by: Helio Perroni Filho <xperroni@gmail.com>
EXPECT_EQ( param::param<std::string>( "string", "" ), "test" );
EXPECT_EQ( param::param<std::string>( "gnirts", "test" ), "test" );
EXPECT_TRUE( param::param<int>( "int", 0 ) == 10 );

This comment has been minimized.

@esteve

esteve Apr 3, 2015

Member

Could you make the EXPECT_EQ change here and for every integer test too? Thanks!

EXPECT_DOUBLE_EQ( nh.param<double>( "double", 0.0 ), 10.5 );
EXPECT_DOUBLE_EQ( nh.param<double>( "elbuod", 10.5 ), 10.5 );
EXPECT_FALSE( nh.param<bool>( "bool", true ) );

This comment has been minimized.

@esteve

esteve Apr 3, 2015

Member

I'd replace this with EXPECT_EQ too. true/false are values returned by nh.param<>(), it's not like it returns a binary answer (as opposed to methods like is_open() or is_valid()), there's a slight semantic difference IMO. @dirk-thomas @tfoote @wjwwood thoughts?

This comment has been minimized.

@wjwwood

wjwwood Apr 3, 2015

Member

In this case I think it doesn't matter. The most important thing is to use EXPECT_EQ on non-bool types so that when they do not match it can print out what the actual versus expected was. In this case there are only to states, so being explicit doesn't help.

Changed assertions used to check int and boolean results
Signed-off-by: Helio Perroni Filho <xperroni@gmail.com>
@xperroni

This comment has been minimized.

Contributor

xperroni commented Apr 3, 2015

Changed test assertions as requested.

@esteve

This comment has been minimized.

Member

esteve commented Apr 3, 2015

@xperroni thank you very much for addressing our comments!

@xperroni

This comment has been minimized.

Contributor

xperroni commented Apr 3, 2015

@esteve My pleasure. :)

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Apr 3, 2015

@ros-pull-request-builder

This comment has been minimized.

Member

ros-pull-request-builder commented Apr 3, 2015

Test passed.
Refer to this link for build results: http://jenkins.ros.org/job/_pull_request-indigo-ros_comm/211/

@dirk-thomas

This comment has been minimized.

Member

dirk-thomas commented Apr 3, 2015

Thank you! Squashed and merged: ba130b9

@dirk-thomas dirk-thomas closed this Apr 3, 2015

@xperroni

This comment has been minimized.

Contributor

xperroni commented Apr 5, 2015

Yay! Thank you for your comments and support. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment