-
-
Notifications
You must be signed in to change notification settings - Fork 96
Add YAML support #1045
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
Add YAML support #1045
Conversation
19fe9fe
to
e7c358c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Second pass.
libvast/src/data.cpp
Outdated
return xs; | ||
} | ||
} | ||
die("unhandled YAML node type"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's pretty harsh, why not just throw YAML::Exception
and fail to parse the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that this is more like a developer issue. We enumerated all switch statements above, so we should never reach this code. If so, it's a logic error. In order to make the compiler happy (and not spit out a warning), I plugged in a [[noreturn]]
function for which we conveniently have die
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with leaving it as-is here, but in general we should be careful with jumping from "programmer error" to "kill the database", especially when there's a perfectly reasonable recovery path.
Related rant: https://lkml.org/lkml/2016/10/4/1
@lava I could not find a single |
35633dc
to
bfb07b0
Compare
I've added a changelog as well. Before the next release, we may consolidate it with a single entry about the new config file format. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please install libyaml-cpp-dev
in the bundled Dockerfile as well.
Why didn't CI complain about this missing dependency, @dominiklohmann? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just cosmetic requests now. Approving already.
Please fix these and wait for CI before merging, especially because of the Docker CI.
@dominiklohmann I pushed a minor fixup and now the Docker build failed with this error:
Any objections if I merge nonetheless? |
I got your approval in person, @dominiklohmann. 🙂 |
To be rebased onto master after #1017 is merged. Ignore everything up to and including 6b4e564.