marzer/tomlplusplus

Key access function that throws on key not found

OlafvdSpek opened this issue · 9 comments

Is your feature request related to a problem? Please describe.

    // different ways of directly querying data
    std::optional<std::string_view> str1 = tbl["str"].value<std::string_view>();
    std::optional<std::string>      str2 = tbl["str"].value<std::string>();
    std::string_view                str3 = tbl["str"].value_or(""sv);
    std::string&                    str4 = tbl["str"].ref<std::string>(); // ~~dangerous~~

    std::cout << *str1 << "\n"; // 1
    std::cout << *str2 << "\n"; // 2
    std::cout << str3 << "\n";
    std::cout << str4 << "\n";
  1. and 2. have undefined behavior if it's a nullopt, .value() might be safer.
    .value() would throw bad optional access, which doesn't tell a lot about the cause or location of the issue.

https://marzer.github.io/tomlplusplus/#mainpage-example-manipulations

Describe the solution you'd like
A key access function that'd throw on key not found with an error message containing the name of the key would be helpful.
A value access function that'd throw on wrong data type might be helpful too.

marzer commented

and 2. have undefined behavior if it's a nullopt

The example code doesn't have any UB because the values exist, so using operator* is safe. The reader understanding this usage of std::optional is an assumption made for the sake of the example.

In normal, non-demo code you should be checking the presence of a value in the returned std::optional, of course. That example doesn't do that because it's more about showing the different ways you can query data, not boring error-handling code we've all written 1000 times and is somewhat out-of-scope. The specific documentation example for value() has that covered already.

There of course will be people who don't understand that std::optional may be nullopt, but that's not really a problem my library needs to address - that is solved simply by reading cppreference.com. std::optional is a vocabulary type and it's use should be encouraged, not avoided.

(as an aside: the library may be used with exceptions disabled completely, so changing value() to rely on exceptions is a non-starter for people relying on that)

marzer commented

Oh, also: toml::table and toml::array both have an at() method that will throw that exception in the same way a std::vector::at() does, so you can actually already get this behaviour if you first get the source node as a table/array:

These are intended for more advanced use, whereas value() 'just works' regardless of what the concrete node actually is, so you can pick your poison.

is an assumption made for the sake of the example.

It's not like using .value() makes the example (significantly) more complex.

so changing value() to rely on exceptions is a non-starter for people relying on that)

I'm not requesting existing semantics to be changed.

marzer commented

I'm not requesting existing semantics to be changed.

Yeah, I initially mis-read your request.

In any case my points about std::optional being a vocabulary type are valid; I could implement something like value_or_throw(), or people could just use std::optional::value() for checked-access semantics, if that's what they need. The standard library already has you covered here for the general case, and for more advanced usage requiring better error mesages this is already implemented via at().

I suppose I could update the example to use std::optional::value(), though I maintain it's out of scope and irrelevant which method is used as the example isn't about teaching people how to use the standard library, but merely demonstrating the different ways to query node values in toml++.

marzer commented

If you genuinely believe the example should be changed to emphasize the specifics of using std::optional here then I'll happily accept a PR, but otherwise I don't think I'll bother.

In any case my points about std::optional being a vocabulary type are valid; I could implement something like value_or_throw(), or people could just use std::optional::value() for checked-access semantics, if that's what they need. The standard library already has you covered here for the general case, and for more advanced usage requiring better error mesages this is already implemented via at().

at() indeed perfectly covers "A key access function that'd throw on key not found with an error message containing the name of the key would be helpful."

What about "A value access function that'd throw on wrong data type might be helpful too." though?
bad optional access doesn't provide enough useful info to the user.

marzer commented

I'll think about it. Having both complicates the implementation quite a bit so I'd rather not.

In the interim if you want a message that says something like value error: tried to get type 'bool' as type 'string' all the tools to do this are in the library now and could be implemented easily in a helper function in your own code.

all the tools to do this are in the library now and could be implemented easily in a helper function in your own code.

True. But if multiple library users have to write the same helper function, it'd make sense to just include it in the library.

marzer commented

it'd make sense to just include it in the library.

What 'makes sense' is up to me. I said I'd think about it, but to reiterate: this implies a very annoying complication of the implementation, so currently I am at a firm "maybe", especially since you're the first user in a number of years to ask for this. The solution I suggested above was an interim solution. Let's leave it at that.