Naming consistency in parsing libraries

[mxgrey]

Right now, we have dart::utils::SkelParser which parses the native DART format .skel, and then we have dart::utils::DartLoader which unintuitively loads .urdf, and then there's dart::utils::SdfParser which is a class for loading SDFs, except all its functions are static, so it's really more of a namespace than a class (the situation for SkelParser is the same).

I would propose the following:

1. kido::utils::skel should be the namespace for the functions that parse the .skel format. The functions should not be in a class named SkelParser at all. This namespace will encapsulate a component library named kido-utils-skel with a dependency on tinyxml2 (related to the discussion here #434).
2. kido::utils::sdf should be the namespace for the functions that parse the .sdf format. Similarly, those functions should not be in a class named SdfParser at all. This namespace will encapsulate a component library named kido-utils-sdf with a dependency on sdformat.
3. kido::utils::urdf should be the namespace for the class kido::utils::urdf::Loader. The urdf parsing still needs a class at the moment, in order to add package directories. The namespace will encapsulate a component library named kido-utils-urdf with a dependency on urdfdom.

I suggest we at least do this for the first major release of KIDO. I don't know for sure whether or not we want to implement this in the next major release of DART as well.

[jslee02]

Overall idea sounds good. Here is a quick note.

SkelParser is changed to a namespace in shapenode branch. Things to do are (1) changing the name of namespace and (2) moving them into skel folder like urdf and sdf.

VskParser should also follow the convention.

The name utils was used to represent more general functionalities including the current files in common directory. As utils now contains only save (and load in the future) functionality, io namespace would be more informative name for the namespace rather than utils.

[mxgrey]

VskParser should also follow the convention.

io namespace would be more informative name for the namespace rather than utils

I like both of those ideas :+1:

[mkoval]

I think we should make DartLoader a collection of static methods as well. You can specify package paths using PackageResourceRetriever. The methods on DartLoader are deprecated and only exist for backwards compatibility - they're thin wrappers around PackageResourceRetriever.

-Mike On Thu, Feb 4, 2016 at 8:46 AM Jeongseok Lee [email protected] wrote:

Overall idea sounds good. Here is a quick note.

SkelParser is changed to a namespace in shapenode branch https://github.com/dartsim/dart/blob/shapenode/dart/utils/SkelParser.h. Things to do are (1) change the name of namespace and move them into a skel folder like urdf and sdf.

VskParser should also follow the convention.

The name utils was used to represent more general functionalities including the current files in common directory. As utils now contains only save (and load in the fuction), io namespace would be more informative name for the namespace rather than utils.

— Reply to this email directly or view it on GitHub https://github.com/dartsim/dart/issues/604#issuecomment-179939844.

[mxgrey]

I agree with removing the need for a class from DartLoader; I just wasn't sure if we wanted to do that immediately. Getting the naming and package management to be consistent before the major release seemed like a higher priority to me, since we can easily deprecate the loader class in the following minor release.

[mxgrey]

I'd suggest we aim to do this refactoring for DART 7.0.

@jslee02 if you don't agree, feel free to remove this from the milestone.

[jslee02]

@mxgrey :+1: