Improve rendering of error messages & fix record to map conversion #1842
Conversation
| @@ -70,7 +70,7 @@ caf::error insert_to_map(To& dst, typename To::key_type&& key, | |||
| else | |||
| // TODO: Consider continuing if the old and new values are the same. | |||
| return caf::make_error(ec::convert_error, | |||
| fmt::format("{}: redefinition detected: \"{}\" " | |||
| fmt::format(": redefinition of {} detected: \"{}\" " | |||
There was a problem hiding this comment.
| fmt::format(": redefinition of {} detected: \"{}\" " | |
| fmt::format("redefinition of {} detected: {} " |
Why not this?
There was a problem hiding this comment.
I am not a fan of beginning the error context with a colon either. If you want to make this a thing @tobim then please make it consistently throughout the code base by adding the colon in the error rendering function.
There was a problem hiding this comment.
I think this is not even an option, because it destroys composition.
There was a problem hiding this comment.
The data conversion overloads are a special case because the message will almost always get prepended with the path that leads to the location of the error. The desired output should be
convert_error: foo.bar[2].baz: redefinition of key detected: "A" vs "B"
I'm open to ideas for how to implement that without the need to add the colon to the inner message.
There was a problem hiding this comment.
You can add the colon when rendering nested errors, just like you special-case std::string in error contexts now.
| @@ -318,7 +327,7 @@ caf::error convert(const list& src, To& dst, const list_type& t) { | |||
| for (const auto& element : src) { | |||
| const auto* rec = caf::get_if<record>(&element); | |||
| if (!rec) | |||
| return caf::make_error(ec::convert_error, "no record in list"); | |||
| return caf::make_error(ec::convert_error, ": no record in list"); | |||
There was a problem hiding this comment.
This seems inside out: adding such context is the job of the calling context, no?
This change removes the insertion of backslashes from log messages that print the content of a `caf::error`. Since errors can be nested some log messages would contain 3 or even 7 consecutive backslashes.
This turns the visitor in the conversion inspector into a double dispatch. Doing so is necessary to catch convert overloads that don't apply to matching data / type pairs. For example record fields that contained another record weren't converted to maps with the correct overload before.
tobim
force-pushed
the
topic/nicer-errors
branch
from
991dbeb
to
08d598e
Compare
tobim
force-pushed
the
topic/nicer-errors
branch
from
08d598e
to
636b22e
Compare
tobim
changed the title
|
@dominiklohmann can you take the review from here? Let's address the |
A recent oversight in #1842 broke the build (again). We must use `VAST_FMT_RUNTIME` for format strings that cannot be verified at compile time, e.g., because they are not constant.
A recent oversight in #1842 broke the build (again). We must use `VAST_FMT_RUNTIME` for format strings that cannot be verified at compile time, e.g., because they are not constant.
📔 Description
Best illustrated with a demo:
Old:
New:
📝 Checklist
🎯 Review Instructions
Commit-by-commit. Should be self-explanatory.