Add Model::initParamWithNodeHandle to branch indigo-devel

[liuzjmike]

No description provided.

[sloretz]

Looks like a cherry pick of pull request #168 to indigo-devel

[liuzjmike]

Yes I think so. I need to deal with multiple robot models in my current program and I think a lot others may have the same need. I thought an easier way might be to add NodeHandle as a parameter with default value ros::NodeHandle() to initParams, but I also felt that I should keep the API in Indigo with that in Kinetic.

[sloretz]

@liuzjmike Mind doing just the cherry pick without the new .gitignore?

[liuzjmike]

@sloretz No problem. Do you want me to push again or could you pick out the .gitignore?

[sloretz]

@liuzjmike It'd be fine to make a new commit that deleted the .gitignore. The PR will get squashed into a single commit anyways, so it will appear in the history as if only Model::initPramWithNodeHandle was added.

[liuzjmike]

Done

[clalancette]

I'm not sure that this is doing the same thing as the kinetic-devel version. There, nh is always initialized with a new ros::NodeHandle() object based on the function signature. Since we aren't doing that here (why not?), in the case where someone just calls initparamWithNodeHandle(), nh is undefined. Am I missing something?

[sloretz]

Ah it is different. In kinetic-devel calling initParamWithNodeHandle("parameter") is identical to initParam("parameter"). In this PR calling initParamWithNodeHandle("parameter") would fail to compile.

I think this is OK since code written for indigo would still work after moving to kinetic. It's only an issue if someone tries to port kinetic code backwards to indigo.

Having a default value for the NodeHandle parameter in kinetic doesn't add value since there is already initParam, but it's too late to change. Maybe the default value could be removed in ROS melodic?

[clalancette]

I see what you mean, yeah. This sequence of events is unfortunate :(. Ideally, we'd actually kill off initParamWithNodeHandle() completely, and just add the default-initialized argument to initParam. We could do that in this PR, but at that point we have Indigo and Kinetic heading off in different directions, which would also suck.

So with all of that in mind (and with what @sloretz said), I guess this is fine as it is.

[clalancette]

So, the basic code is simple and fine. What I think I'm missing here is unit tests to cover the new initParamWithNodeHandle method. Would you mind adding that to the tests? Thanks.

[clalancette]

I see what you mean, yeah. This sequence of events is unfortunate :(. Ideally, we'd actually kill off initParamWithNodeHandle() completely, and just add the default-initialized argument to initParam. We could do that in this PR, but at that point we have Indigo and Kinetic heading off in different directions, which would also suck.

So with all of that in mind (and with what @sloretz said), I guess this is fine as it is.

[liuzjmike]

@clalancette I'm sorry but I am not familiar with writing unit tests for ROS or in C++ in general. Also I assume that since kinetic already has the same method, it should be safe to add it in indigo? The only dependence on nh in the code is from nh.searchParam and nh.getParam, which should perform the same way in both kinetic and indigo.