Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better support for "default" #973

Open
kunaltyagi opened this issue Jan 21, 2021 · 2 comments
Open

Better support for "default" #973

kunaltyagi opened this issue Jan 21, 2021 · 2 comments

Comments

@kunaltyagi
Copy link

kunaltyagi commented Jan 21, 2021

YAML is often used for configuration input, and it's a bit difficult to manage defaults:

Current:

inline bool validField(const YAML::Node& node, const std::string& field) noexcept {
    if (node.IsDefined() && node[field].IsDefined() && !node[field].IsNull()) {
        return true;
    }
    return false;
}

// The convert struct is helpful only once: for reading the defaults
// default = node.as<Cfg>();

Cfg readConfig(const YAML::Node& node, const Cfg& default) {
    Cfg config = default;
    if (validField(node, "first")) {  config.first = node["first"].as<decltype(config.first)>();
	// ...
	// repeat for N number of fields
	return config;
}

Proposed:

Cfg readConfig(const YAML::Node& node, Cfg default) {
	// Note the change from const reference to value for default
	node.into<Cfg>(default);  // 100% usage of convert struct
	return default;
}

Roadmap:

  • Add templated update function to Node. Bikeshedding on name is on the table. into just reads nice
  • Update the as_if struct with a function, say update, in node/impl.h to not create the default variable
  • The into (newly added templated function) calls the update function.

Proposed change in as_if:

template <typename T, typename S>
struct as_if {
  explicit as_if(const Node& node_) : node(node_) {}
  const Node& node;

  // current, can be replaced with the newer function
  T operator()(const S& fallback) const {
    if (!node.m_pNode)
      return fallback;

    T t;
    if (convert<T>::decode(node, t))
      return t;
    return fallback;
  }

  // new
  bool update(S& incoming) {
	if (!node.m_pNode)
	  return false;
	if (convert<T>::decode(node, incoming))
	  return true;
	return false;
  }

  // compilers are smart enough to reorder `T t` after the inlining of `node.m_pNode` check
  T operator()(const S& fallback) const {
	T t;
	if (update(t))
	  return t;
	return fallback;
  }	
};

I can create a PR if needed, but I'm confused about the overloads for S = void case

@W4RH4WK
Copy link

W4RH4WK commented Aug 7, 2022

Hey there, I am running into a similar problem at the moment and just found this. However, I'd like to have this implemented a bit different.

Leave Node::as and the as_if functor as they are. I think the behavior of those is fine and doesn't need to be changed.

Add Node::into<T>(T&) which decodes a node into the given instance of type T. It's implemented using the same pattern as as_if is to Node::as. I.e. a dedicated functor (if this is even necessary) checking validity and invoking the decode function.

@W4RH4WK
Copy link

W4RH4WK commented Aug 7, 2022

For future reference, until a Node::into<T> or equivalent is merged, this seems to completely resolve my use-case:

template <typename T>
bool decodeInto(const YAML::Node& node, T& object)
{
	return node && YAML::convert<T>::decode(node, object);
}

What was originally:

auto object = node.as<Object>(); // <-- internally default-constructed

Becomes:

Object object;
// initialize object as needed

bool ok = decodeInto(node, object);

convert<Object>::decode remains unchanged.


Note that this decode into method allows me to de-serialize to objects that exist before YAML::load is called. Now I don't have to construct new objects in order to de-serialize data.


Related #993/#1087: This PR tackles the same problem and proposes a solution where the object is constructed inside the decode function. I advice against this as I deem constructing the object outside the decode function to be far easier in the case where the object cannot be sensibly default-constructed. In order to construct the object inside the decode function, all information necessary to construct the object needs to be forwarded into the decode function somehow. This might not be trivial.

Related #1010: A similar PR, tackling the same problem. And like with #1087, I see issues with constructing the object inside the de-serialization logic. The example shown in #1010 seems fine as long as all information necessary to construct the object can be found in the given node (or sub-nodes). But if needed information is not available there, constructing the object becomes more complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants