API review

Proposer: Jack O'Quin

Present at review:

  • Chad Rockey
  • Tully Foote
  • Michael Carroll
  • Eric Perko

Interfaces

Interfaces to review:

Release Plan

The entire unique_identifier stack will be released in its current form for Fuerte.

In Groovy, the uuid_msgs package will migrate to common_msgs, making it convenient for future uses to depend on that message. The unique_id package will become a unary stack, released separately. In Groovy (and maybe Hydro) the unique_identifier stack will become an empty meta-stack, depending on common_msgs and unique_id for backwards compatibility.

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.

  • (Chad) I looked through the APIs and the message. I haven't used UUIDs a lot in the past. The message looks good and consistent with the definitions. The conversion and helper functions look complete in both C++ and Python, the conversions between Boost and the Python types look especially convenient and should mesh well with any existing applications.

  • (Jack) I chose the 16-byte binary representation in the uuid_msgs/UniqueID message instead of the string representation. Strings are much better for humans reading the messages, but my experiments with Open Street Map revealed the potential for messages containing hundreds of thousands of UUIDs. For that, the 16-byte form beats a 36-character string. The unique_id package provides convenient toString() functions to help us humans.

    • (Tully) Keeping things simpler is better. The availability of boost and python native conversions is great. And I agree with the binary format, and UUIDs aren't "human readable" anyway why make the data format human readable.

  • (Michael) Looked through APIs and messages. Like Chad, I haven't had much opportunity to use UUIDs, but agree that the interface is well-written and clean. It appears that everything was clearly thought out and documented.

    • (Eric) Actually, I think we need to remove a few things from the C++ public facing namespace. If you look at the C++ API docs you'll notice some things that could be confusing. For example, the genURL object is actually listed in the function list (but after looking at the source it's obviously not a function). Also, I'm not sure why some of those internal variables (genURL, genRandom, etc) are exposed. I see two options: a class (e.g. !UUIDGen ) with all of the functions that depend on those variables as static members (and therefore the variables could be marked private...) or we just hide them in something like an impl or detail namespace so you wouldn't accidentally think they were part of the official API (e.g. unique_id::genRandom becomes unique_id::impl::genRandom).

    • (Jack) genUrl actually is a function. All of the gen* names are functions. Doxygen treats that one as a function because of its syntax (I think). But, all of the generators actually instantiate function objects. Some of them may be useful, I suspect, but hiding them in an impl object still seems reasonable.

  • (Jack) I ran the pre-release tests, and discovered to my dismay that the boost uuid package is much newer than I had realized:

    • boost 1.40.0 (Lucid) has no uuid support. That is a serious problem for Fuerte.

    • boost 1.42.0 (Natty) has uuid, but without string output conversions.
    • boost 1.46.1 (Oneiric) has everything we need.
    • All the Groovy platforms have adequate versions.
  • (Jack) I am considering some options for handling the lack of Lucid support. Suggestions are welcome.

    • Use copies of the boost/uuid headers when the system boost is too old (messy and a hassle for maintenance).

      • (Eric) -1. I'd prefer just saying "Only supported in Groovy or newer" over this approach.

    • Only provide the Python APIs before Groovy (I am leaning this way).

      • (Eric) This is better than the boost/uuid headers option. I think the best plan is to split the unique_id package into a unique_id_cpp and a unique_id_python package. That way, a Fuerte user (or older) could checkout the unique_id_python and uuid_msgs package and be happily on their way without having to do anything special to avoid compiling the C++ API on an incompatible OS.

      • (Jack) Good idea. Very few applications need both the C++ and the Python APIs.

Meeting agenda

The final review meeting will be held via e-mail next Friday, 2012-07-13.

Please add your comments to this wiki page before then.

Conclusion

  • /!\ Do not release C++ API in Fuerte due to lack of boost::uuid support in Lucid.

  • /!\ Include C++ API in Groovy.

  • /!\ Hide boost C++ implementation details.

  • /!\ Add boost::uuids::uuid unique_id::fromHexString(const std::string &str) to C++ API.

  • /!\ For both languages rename toString() functions to toHexString()

  • /!\ Move uuid_msgs to common_msgs stack in Groovy.


Wiki: unique_identifier/Reviews/2012-07-06_API_Review (last edited 2012-07-16 22:17:49 by JackOQuin)