Get on non-existing key will panic

[liubin]

Originally posted by @gnprice in #13 (comment):

Interesting, can you say more about the motivation for this subcommand [toml check]?

I think the way I'd expect to handle this use case in a CLI is by using the "get" command, and just redirecting the output to /dev/null if I don't actually want it.

Looking at git config — which I find to be a good source of inspiration for this command's interface, as mentioned in the README — it looks like that's exactly what it does. There's no specialized "check" action in git config --help, but instead there's this:

   --get
       Get the value for a given key (optionally filtered by a regex matching the
       value). Returns error code 1 if the key was not found and the last value if
       multiple key values were found.

I think that'd work well for the toml command, too.

---

Looks like our current behavior in toml get if there's no data at the given path is to panic, eep:

$ target/debug/toml get Cargo.toml a.b ; echo $?
thread 'main' panicked at 'attempted to leave type `linked_hash_map::Node<alloc::string::String, table::TableKeyValue>` uninitialized, which is invalid', /home/greg/.cargo/registry/src/github.com-1ecc6299db9ec823/linked-hash-map-0.5.2/src/lib.rs:174:52
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
101

Or after upgrading dependencies, the panic is a bit less noisy but still a panic:

$ target/debug/toml get Cargo.toml a.b ; echo $?
thread 'main' panicked at 'index not found', src/main.rs:188:32
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
101

It's panicking in the walk_tpath function.

So I think basically what we want here is to take your check_exists function [from #13], which is a non-panicky version of walk_tpath, and put that logic in walk_tpath so that toml get can give a clean error result (just like git config does) instead of panicking.

[gnprice]

OK, fixed! And released the fix, in v0.2.3.

Thanks again @liubin for the report.

[gnprice]

A little bit more history on this issue, history that I (re-)discovered while doing the small survey of uses of toml-cli that I mentioned at #7 (comment) :

In toml-cli v0.2.0, which was the latest release for some time, the behavior was different — toml get of a non-existing key would print "null", and exit with success:

$ cargo install -q --root . [email protected]
$ ./bin/toml get foo.toml a.b ; echo $?
null
0

That behavior isn't awful. It's the same behavior as jq has:

$ toml get foo.toml . | jq '.a.b' ; echo $?
null
0

But it wasn't great either. It's fine for jq in part because jq is for JSON, and null is a perfectly good value in JSON; but there's no such thing in TOML, which makes it an awkward fit for toml get. Also sometimes you do really want to know whether the value was there or not; for those situations, jq has an option -e/--exit-status which makes it exit with failure in this case, and toml get did not have such an option.

Then in commit 9c4fe0b, I upgraded the toml_edit dependency from 0.3.1 to 0.12.6. That changed the behavior of looking up a key that doesn't exist, and so the behavior of toml get changed from "null"/success to the panic described above. At that point there wasn't much in the way of tests — those came a bit later, on the way to v0.2.3 — so I didn't realize the behavior had changed. The panic behavior was first released in toml-cli v0.2.1.

---

I'm happy with the new git config-like behavior, for the reasons above:

$ toml get Cargo.toml a.b; echo $?
1

So I don't plan to change it back to printing "null". But I will add a mention of this to the v0.2.1 changelog, now that I know about the change.