= API review =

Proposer: '''kwc'''

Present at review:

 * Eric
 * Tim
 * Wim
 * Bhaskara
 * Tully
 * Josh
 * Eitan


'''Please review the [[rosh/Overview|ROSH Overview documentation]] for this review.'''
 * Features marked '''EXPERIMENTAL''' will not be considered stable in ROS 1.4 but are open to review
 * Features not documented will not be considered stable in ROS 1.4 but are open to review, assuming you know what they are
 * ROSH plugins will be reviewed separately, though API for loading plugins is part of the review


I am hoping for:

 * general user feedback: what works, what doesn't.
 * what could make rosh a better shell?
 * what could make rosh a better rospy replacement?
 * is the rosh API consistent and friendly?
 * what critical features are missing?

ROSH will be included in the "ros_comm" stack for ROS 1.4. The "stable" version of this API will be released against C Turtle, then rosh will be forked for final Diamondback integration. 


== Question / concerns / comments ==
Enter your thoughts on the API and any questions / concerns you have here.  Please sign your name.  Anything you want to address in the API review should be marked down here before the start of the meeting.

 * Eric
  * Most of my feature requests are already ticketed.
  * The topics[0] vs. topics[1] syntax was definitely confusing
  * Major things that I found myself dropping out of rosh to do when it was my default shell:
   * Non ros-related things (e.g. editing files).
   * rostopic echo XXX | grep YYY or "rostopic list | grep rotated"
   * launching .launch files
   * running guis (e.g. rosrun dynamic_reconfigure reconfigure_gui)
  * Automatically importing rospy into the namespace would be nice.  I found myself importing rospy to use various features (e.g. rates) much more frequently than I found myself importing any other components.
  * Similarly, importing roslib.load_manifest into the namespace by default would make grabbing one function from python code significantly quicker.
  * The other biggest problem I had was with the distinctions in various places between usable objects and informational objects (which print out right, but can't be used, such as with parameters).  I don't know what the solution is here, but clear messages (such as the updated ones for parameters) help a lot here.

 * bhaskara
  * what does rostype return for a service, parameter, or action?
    * kwc: `ServiceDefinition`, nothing, and TBD (actions out of scope, but whatever type class is the best index)
  * why is getting a parameter done via a function call, but setting one done via an assignment operator?
    * kwc: It's an artifact of what tab-completion will allow. In order to descend a namespace, I have to used a boxed object, which means you have to call a function to unbox. 
  * Is dictionary assignment to a parameter namespace additive or overwriting?
    * kwc: overwriting
  * would be nice to have a wait_for_service
    * kwc: two options. (1) I explicitly expose this. (2) I make it the default behavior that rosh always waits for service on a service call.
  * is there an advertise_topic?
    * kwc: no, you advertise by publishing. Also, the default mode for publishing is latching.
  * So the example of launching nodes would be an error due to duplicate names, right?
  * rostype(obj, val): could this be set_rostype(obj, val), or obj.rostype=val ?
    * (kwc) `set_rostype`: more characters, but yes
    * (kwc) `obj.rostype = val`: no, it can't, because that would make 'rostype' a reserved name
  * Would be cool to not have all name references be global, e.g., have a current_namespace global variable (or a 'with Namespace("foo/bar")' syntax)
    * (kwc): Interesting. It might be possible to pull off with thread locals, but it comes at additional lookup expense. I wouldn't do it as a global variable, because that mucks up multithreaded code (smach has this issue). As an alternative, you get the same effect (though only per concept) by using local variables: {{{
ns = parameters.foo.bar
ns.x = 1
ns.y = 2

or
ns = 'foo/bar'
parameters[ns].bar
topics[ns].foo
}}}

 * kwc:
  * `autoheader(frame_id='base_link')`
   * kwc: should the default `frame_id` be empty instead?
   * bhaskara: I think usage patterns vary pretty widely, so there's no one frame_id that would be a great default.

  * `topics[0] vs. topics[1]`
   * kwc: do people like having a non-blocking `topics[0]`, or is it not worth the confusion?
    * bhaskara: I'd find this useful.

 * gerkey:
  * Would be nice to have an explicit subscribe, to allow me to prepare to receive a message to be published in the future (e.g., an action result, if I'm poking an action server manually).  I can get the same behavior via `topics.foo[0]` and discarding the result, but that doesn't feel natural to me.
    * kwc: I'll look into exposing this functionality
  * Would be nice to be able to easily check the publication rate of a topic, e.g., `hz(topics.foo)` or `hz('foo')`.  If I can discover that `foo` is being published at 0.5Hz, then I'll know not to ask for `topics.foo[0:100]` unless I'm very patient.
    * kwc: this is on the todo list, though separately I need to fix things so topics.foo[0:100] returns immediately.
  * The docs for `topics.foo[N]` look like an inadvertent copy-n-paste from `topics.foo[1]`.
  * Is there anything to be said about roshlets?
    * kwc: I've moved the roshlets documentation into the Overview
 
 * joq:  
  * This looks like a very useful idea, but I was unable to figure it out well enough to review the API. 
  * For an outsider like me, the overview documentation needs an introduction explaining what this package does and how it fits into the system. I think it's an interactive python shell for access to ROS interfaces. It would help to know how it relates to `rospy`, whether it requires a running `roscore`, etc.
  * The Install doc could use an explanation of what to install for running it in `cturtle` and in `unstable`. That would help more users try it out.
  * I look forward to running this.
    * (kwc): thank you for the documentation comments. I'll definitely take care of these before the doc review.

== Meeting agenda ==
To be filled out by proposer based on comments gathered during API review period

== Conclusion ==
Package status change  mark change manifest)
 * /!\ Action items that need to be taken.
 * {X} Major issues that need to be resolved

API conditionally cleared:

 * /!\ import `rospy` by default [DONE]
 * /!\ changed `topics[0]` to `topics[-1]` (`topics[LATEST]`) [DONE]
 * /!\ add explicit subscribe [DONE]
 * /!\ move `autoheader` to `rosh_geometry`, possibly call `nowheader` or `header` [DONE - header]
 * /!\ add `wait_for_service` [DONE]
 * /!\ wait for services by default [DONE]

'''Fixes completed 2/13/2011'''

----
## PackageReviewCategory