add helper methods for accessing params #233

gerkey · 2016-07-01T19:48:07Z

Add features that are handy when working with params:

Check for existence of param: bool exists = client.has_parameter("foo")
Get value of one param: double bar = client.get_parameter<double>("foo") (throws if the param isn't set)
Get value of one param, with default: double bar = client.get_parameter("foo", 4.2) (returns 4.2 if the param isn't set)

Linux:
OS X:

dirk-thomas · 2016-07-01T20:09:08Z

rclcpp/src/rclcpp/parameter_client.cpp

+{
+  std::vector<std::string> names;
+  names.push_back(parameter_name);
+  auto vars = get_parameters(names);


Getting the parameter value only to check if the parameter exists if wasteful. Consider a parameter which stores a large amount of data.

dirk-thomas · 2016-07-01T20:10:22Z

I would suggest to keep core functionality separate from syntactic sugar and helper API. The first was is crucial to implement a features while the second is only providing additional interfaces.

gerkey · 2016-07-01T20:39:30Z

@dirk-thomas What do you mean by "separate"?

dirk-thomas · 2016-07-01T20:45:51Z

It should be easily possible to ignore all the syntactic sugar API for testing / validation / review. Separate files would be good in my opinion.

dirk-thomas · 2016-07-01T20:49:22Z

The patch introduces new warnings on Windows: http://ci.ros2.org/job/ci_windows/1589/warnings35Result/new/

gerkey · 2016-07-01T20:57:45Z

The new functions (get_value() and has_parameter()/get_parameter()) are class methods (in ParameterVariant and SyncParametersClient, respectively). How should I go about putting them in separate files (without moving them out of those classes, which would just make them harder to use)?

gerkey · 2016-07-01T21:01:32Z

Thanks for the tip on the Windows warnings. They can be fixed in the future. We removed Windows from our standard CI to allow us to move faster.

gerkey · 2016-07-01T21:17:25Z

The new tests are failing for FastRTPS, though in a manner that I don't think is related to what I've added. I'm seeing this:

[ RUN      ] test_local_parameters__rmw_fastrtps_cpp.helpers
/home/gerkey/ros2_ws_debug/src/ros2/system_tests/test_rclcpp/test/test_local_parameters.cpp:130: Failure
Expected: baz = parameters_client->get_parameter<double>("baz") doesn't throw an exception.
  Actual: it throws.
/home/gerkey/ros2_ws_debug/src/ros2/system_tests/test_rclcpp/test/test_local_parameters.cpp:131: Failure
Value of: 1.45
Expected: baz
Which is: 0
[rmw|error_handling.c:93] error string being overwritten: cannot publish data, at /home/gerkey/ros2_ws_debug/src/eProsima/ROS-RMW-Fast-RTPS-cpp/rmw_fastrtps_cpp/src/functions.cpp:1619, at /home/gerkey/ros2_ws_debug/src/ros2/rcl/rcl/src/rcl/client.c:170

If I comment out that block of tests (https://github.com/ros2/system_tests/blob/param_helpers/test_rclcpp/test/test_local_parameters.cpp#L128-L133), then a similar failure appears on the following block of tests. And if I comment out the previous block of tests (https://github.com/ros2/system_tests/blob/param_helpers/test_rclcpp/test/test_local_parameters.cpp#L121-L126), then the failure also moves to the following block.

I'm wondering if there's some buffering issue that's presenting itself here because of the long sequence of parameter service calls. Anybody know of a problem doing lots of service calls with FastRTPS? I'm not seeing this on OpenSplice and CI doesn't report failures on Connext.

dirk-thomas · 2016-07-01T21:29:25Z

rclcpp/include/rclcpp/parameter.hpp

+
+  template<typename type>
+  typename std::enable_if<std::is_integral<type>::value && !std::is_same<type, bool>::value,
+  int64_t>::type


Please revise the indentation here since it is really hard to read.

Please make a specific suggestion for how to revise it. The current version passes the linter and I'm not able to run your hard-to-read detector locally.

The common rule says: everything with in block ((, {, <) should be indented one level further in order to make it distinguishable.

In this case you can do e.g.:

typename std::enable_if< std::is_integral<type>::value && !std::is_same<type, bool>::value, int64_t>::type

or

typename std::enable_if< std::is_integral<type>::value && !std::is_same<type, bool>::value, int64_t>::type

OK, I did that in ecdc01b.

But we've got to figure out a better way to handle style. It's frustrating and unproductive to have the style rule be: "make our incredibly picky linters happy, but then also make whatever changes are preferred by other developers, according to various unwritten rules."

Technically both before and after styles were "correct", as they passed our linter and guidelines, but the new version you committed is more readable, at least in my opinion. Unfortunately getting a linter to tell you when something could be more readable is really hard, and figuring out how to make code look readable and still remain concise is a little bit of an art form. To that end, I think as much as possible in these cases should be left to the developer's discretion. In this particular case, I'd say what you originally did was fine. But I don't see any reason that one developer can't suggest an alternative syntax that is easier to read or understand, which is what I'd say is happening here. @dirk-thomas may see it differently, but you could refuse to change it too, if you like. I'd also say this one of those situations where empathy for the other developer would suggest that you make a pr to do the change for the original developer. That way it can be either merged or rejected, but it minimizes effort and frustration on the part of the original developer. At the very least, starting with a specific suggestion on how to change it and some rational why you think it's better.

<pedantry>
Also, @gerkey I'll have to disagree with your commit message 😄, if there is a style standard for a project or language (e.g. PEP8), then there is such thing as a correct and an incorrect style in certain contexts. One could say that there's no universally correct style or that correct style is is always in the context of a style standard. One could also say that there is no objectively superior style standard, but that's a different thing.
</pedantry>

In this specific case (indenting blocks of code) I think it doesn't really matter what the linter allows beside it. This rule is common across almost all style guides I have ever seen. In my opinion not every code passing our linters is "good to be merged". I could make numerous examples which linters won't catch but would look pretty bad - hopefully others would agree.

Also implying that any kind of style changes can be left for other peoples is not a good proposal. Simply because that means when creating a pull request you have the "freedom" to do whatever passes the linters and leave the
cleanup to other people.

With the goal in mind to develop well readable, consistent and maintainable source code we should follow rules (even when not covered by a linter). If some are not documented we should try to do that so that everybody is on the same page.

I like the idea of putting the burden on the reviewer to specifically propose non-essential style changes, with the original developer having the authority to make the final call.

I think that's ok so long as the the developer tries to avoid the need for such changes over time. I think it will work out better if both sides are more generous with their effort.

I could make numerous examples which linters won't catch but would look pretty bad - hopefully others would agree.

I'm pretty sure that in any of those cases, I might prefer it to be different or think that it looks strange, but I wouldn't label it as wrong or prevent a merge based on that unless it's a functionally dangerous style (e.g. if-statement without brackets). I think it's unreasonable to require style enforcement which is not covered by the linter. I think it is reasonable to ask people learn the team's prefered styles that aren't covered by the linter over time and to be open minded to style changes when they are suggested during reviews.

Also implying that any kind of style changes can be left for other peoples is not a good proposal.

What I was implying was that it would be courteous to offer to fix style changes, either because the original developer may disagree with you or because they're not aware of a potentially better way to do it. And in both cases you're likely to get a more positive response if you act more humbly and respectfully. Hopefully they will agree that the proposed style is better, and in the future they'll apply these suggestions to their own original work and there won't be any need for further pr's from reviewers. As you suggested, we can also capture these best practices that aren't covered by the linter in one of our online documents, if they aren't already. But to require someone, especially a less frequent contributor, to "just know" is unreasonable imo, and if you insist that they fix every little item you see it's just going to frustrate them further.

Simply because that means when creating a pull request you have the "freedom" to do whatever passes the linters and leave the cleanup to other people.

I've never heard the word freedom used in such a negative tone... 🇺🇸 Everyone should try to get the style in a reasonable state on the first try so others don't feel as if they have to "cleanup" after someone, but if they don't realize it and the linter didn't tell them, then I don't see how we can require it of them. When we think it could be better, then we should offer to help, not demand it should change in exchange for allowing it to be merged.

With the goal in mind to develop well readable, consistent and maintainable source code we should follow rules (even when not covered by a linter). If some are not documented we should try to do that so that everybody is on the same page.

I agree with most of that, except that we should be patient and lenient with style that isn't covered by the linter, i.e. "ask" not "tell", especially for some of the more subjective changes. In general I think that a more courteous attitude on the reviewer's side and an open minded attitude on the developer's side would go along ways to accomplishing those goals.

Style is debatable, and while we generally defer to the linter, nobody gets a veto. When there's a question as to what's preferred, we should default to give preference to the person who took the time to write the code.

+1, @wjwwood

I was cut off getting on a plane: we can't go wrong by being more open-minded, generous, and courteous.

gerkey · 2016-07-01T21:37:43Z

I ticketed the FastRTPS problem, along with a simpler test that demonstrates it: ros2/rmw_fastrtps#46.

I'm going to work around that issue in this PR.

dirk-thomas · 2016-07-01T22:40:33Z

@gerkey See #234.

…t style)

gerkey · 2016-07-01T23:15:14Z

New CI, with known failing tests disabled for FastRTPS:

Linux:
OS X:

gerkey · 2016-07-06T23:02:27Z

The connext failures can be explained by ros2/rmw_connext#193, which I worked around in ros2/system_tests@28e1623.
The fastrtps failures will be fixed when #236 is merged. I'll wait for that, then re-run CI here.

* use the param profile for param services * expose qos to users of param clients / services

…t style)

…aram_helpers

gerkey · 2016-07-08T20:54:18Z

Replaced by #237, with same diff.

* add parameter helpers * respond to comments * remove unnecessary indent comments * replace temp variable assignment with explicit constructor invocation

add helper methods for accessing params

ed25310

gerkey added the in progress Actively being worked on (Kanban column) label Jul 1, 2016

gerkey mentioned this pull request Jul 1, 2016

add tests for new param helpers ros2/system_tests#149

Closed

gerkey self-assigned this Jul 1, 2016

gerkey added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jul 1, 2016

dirk-thomas reviewed Jul 1, 2016
View reviewed changes

list instead of get param when checking for existence

c48fb0e

dirk-thomas reviewed Jul 1, 2016
View reviewed changes

dirk-thomas and others added 2 commits July 1, 2016 15:44

fix warnings (#234)

215506a

change style (not fix style, becaused there's no such thing as correc…

ecdc01b

…t style)

gerkey mentioned this pull request Jul 6, 2016

Do something about name length limits ros2/rmw_connext#193

Closed

gerkey and others added 6 commits July 8, 2016 13:39

use the param profile for param services (#236)

ea76716

* use the param profile for param services * expose qos to users of param clients / services

add helper methods for accessing params

1540e80

list instead of get param when checking for existence

4c9a028

fix warnings (#234)

b539c76

change style (not fix style, becaused there's no such thing as correc…

df1ef5c

…t style)

Merge branch 'param_helpers' of https://github.com/ros2/rclcpp into p…

8514f26

…aram_helpers

gerkey mentioned this pull request Jul 8, 2016

add parameter helpers (redo of #233) #237

Merged

gerkey closed this Jul 8, 2016

gerkey deleted the param_helpers branch July 8, 2016 20:54

gerkey removed the in review Waiting for review (Kanban column) label Jul 8, 2016

gerkey added a commit that referenced this pull request Jul 8, 2016

add parameter helpers (redo of #233) (#237)

fc0d539

* add parameter helpers * respond to comments * remove unnecessary indent comments * replace temp variable assignment with explicit constructor invocation

dirk-thomas mentioned this pull request Jul 29, 2016

Get single parameter #245

Merged

Karsten1987 pushed a commit that referenced this pull request Dec 7, 2016

add parameter helpers (redo of #233) (#237)

00f0829

* add parameter helpers * respond to comments * remove unnecessary indent comments * replace temp variable assignment with explicit constructor invocation

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add helper methods for accessing params #233

add helper methods for accessing params #233

gerkey commented Jul 1, 2016 •

edited

Loading

dirk-thomas Jul 1, 2016

gerkey Jul 1, 2016

dirk-thomas commented Jul 1, 2016

gerkey commented Jul 1, 2016

dirk-thomas commented Jul 1, 2016

dirk-thomas commented Jul 1, 2016

gerkey commented Jul 1, 2016

gerkey commented Jul 1, 2016

gerkey commented Jul 1, 2016

dirk-thomas Jul 1, 2016

gerkey Jul 1, 2016

dirk-thomas Jul 1, 2016 •

edited

Loading

gerkey Jul 1, 2016

wjwwood Jul 1, 2016

dirk-thomas Jul 2, 2016

wjwwood Jul 2, 2016

gerkey Jul 2, 2016

gerkey Jul 2, 2016

gerkey Jul 2, 2016

gerkey commented Jul 1, 2016

dirk-thomas commented Jul 1, 2016

gerkey commented Jul 1, 2016

gerkey commented Jul 6, 2016

gerkey commented Jul 8, 2016

add helper methods for accessing params #233

add helper methods for accessing params #233

Conversation

gerkey commented Jul 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Jul 1, 2016

gerkey commented Jul 1, 2016

dirk-thomas commented Jul 1, 2016

dirk-thomas commented Jul 1, 2016

gerkey commented Jul 1, 2016

gerkey commented Jul 1, 2016

gerkey commented Jul 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas Jul 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gerkey commented Jul 1, 2016

dirk-thomas commented Jul 1, 2016

gerkey commented Jul 1, 2016

gerkey commented Jul 6, 2016

gerkey commented Jul 8, 2016

gerkey commented Jul 1, 2016 •

edited

Loading

dirk-thomas Jul 1, 2016 •

edited

Loading