Correct usage of YAML::convert on restricted maps.

[adrianbroher]

Hello, I'm currently testing out this library for usage in another project and I'm wondering if the current implementation matches the intended usage of YAML::convert. I think my current usage is more verbose than it needs to be (intentionally leaving out some code here):

planets.yml

planets:
  swamp:
    - surface: planets/Swamp01.png
      shininess: 0.0
      atmospheres:
        - # left out for brevity
  toxic:
    # sequence of planet data objects

Datastructure

    struct PlanetData {
        struct Atmosphere; // definition left out for brevity.

        std::string filename;
        float shininess;
        std::vector<Atmosphere> atmospheres;
    };

Loading code

            YAML::Node doc;
            try {
                boost::filesystem::ifstream ifs(ClientUI::ArtDir() / "planets" / "planets.yml");
                doc = YAML::Load(ifs);
                ifs.close();
                data = doc["planets"].as<std::map<PlanetType, std::vector<PlanetData>>>();
            }

            catch(YAML::TypedBadConversion<std::map<PlanetType, std::vector<PlanetData>>>& e) {
                // ... error message "line:column: expecting a map of planet types to sequence of planet data objects"
            }
            catch(YAML::TypedBadConversion<std::vector<PlanetData>>& e) {
                // ... error message "line:column: expecting a sequence of planet data objects"
            }
            catch(YAML::TypedBadConversion<PlanetData>& e) {
                // ... error message "line:column: expecting a planet data object"
            }
            // ... more YAML::TypedBadConversion<PlanetType> catch blocks
            // ... more YAML::TypedBadConversion<PlanetData::Atmosphere> catch blocks
            catch(YAML::Exception& e) {
                std::cerr << e.mark.line << ":" << e.mark.column << ": " << e.what();
            }
        }

Custom type converter

namespace YAML {
    template <>
    struct convert<PlanetData> {
        static bool decode(const Node& node, PlanetData& rhs);
    };

    bool convert<PlanetData>::decode(const Node& node, PlanetData& rhs) {
        if (!node.IsMap())
            return false;

        if (!node["surface"])
            throw YAML::KeyNotFound(node.Mark(), std::string{"surface"});

        if (!node["shininess"])
            throw YAML::KeyNotFound(node.Mark(), std::string{"shininess"});

        if (3 == node.size() && !node["atmospheres"])
            throw YAML::KeyNotFound(node.Mark(), std::string{"atmospheres"});

        if (3 < node.size())
            throw YAML::RepresentationException(node.Mark(), "unexpected attributes in planet object aside from: [surface, shininess, atmospheres]");

        rhs = {
            node["surface"].as<std::string>(),
            std::max(0.0f, std::min(node["shininess"].as<float>(), 128.0f)),
            node["atmospheres"].as<std::vector<PlanetData::Atmosphere>>(std::vector<PlanetData::Atmosphere>{})
        };

        return true;
    }

    // leaving out convert<PlanetType> and convert<PlanetData::Atmosphere>
}

For now I have several problems with this code and I'm not sure if those stem from improper use or are bugs:

• I would assume calling node["missing_key"].as<some_type>() would automatically raise a YAML::KeyNotFound exception with a valid node.Mark() value. Instead a YAML::InvalidNode with "missing_key" as key set, but a null node.Mark() value, making it impossible to trace the wrong line. It also seems like YAML::KeyNotFound isn't used anywhere. Would it be better to introduce a YAML::Node::at(key) method to add a boundary check similar to std::map::at(key)?
• The catch blocks around the YAML::Load and initial node["root_key"].as<structure>() need a separate catch block for each YAML::TypedBadConversion<T> to get a more meaningful message instead of "bad conversion". This becomes very complex pretty fast depending on the data structure built. Should I ignore the return value of convert::decode (and TypedBadConversion) and instead raise YAML::RepresentationException instead whenever the conversion cannot occur? What about the builtin conversions for std containers?
• The YAML::KeyNotFound constructor discards the key when it is only a plain string literal, making the error message unusable.
• The YAML::Exception::what method doesn't contain the row or column where the exception occurred.

[jbeder]

There are some usability issues with error handling that you've run into.

1. The YAML::InvalidNode should probably have the node.Mark() properly set; I don't remember what YAML::KeyNotFound was.
2. The what() should have more information, right? What kind of more information are you looking for that you wouldn't need a TypedBadConversion<T> for instead?
3. This sounds like a bug.
4. This sounds like a bug.

[adrianbroher]

1. The YAML::InvalidNode should probably have the node.Mark() properly set; I don't remember what YAML::KeyNotFound was.

The KeyNotFound and TypedKeyNotFound are defined in yaml-cpp/exceptions.h but are not instantiated anywhere directly or via MakeTypedKeyNotFound function.

2. The what() should have more information, right? What kind of more information are you looking for that you wouldn't need a TypedBadConversion<T> for instead?

Ideally I would expect a what() message similar to Expected a TypeName when the decode<TypeName> raises an exception or returns false, which in turn emits a TypedBadConversion<TypeName>. I also believe those exception should be written for immediate human consumption (log files, error messages) without insight into the emitting source code, so convert struct maybe should have a dedicated type_name string literal which contains the pretty name of the converted type.

Given the previous code:

    template <>
    struct convert<PlanetData> {
        static bool decode(const Node& node, PlanetData& rhs);

        static char const* type_name = "Planet model";
    };

The emitted what message for convert<PlanetData>decode returning false should look like:

Expected a Planet model

When the decode function emits an exception (here for a missing key called surface) I would suggest the following message:

Failed to decode Planet model: missing key "surface".