-
Notifications
You must be signed in to change notification settings - Fork 221
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 Dhall support #218
Add Dhall support #218
Conversation
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.
Hi,
Awesome, thanks for your contribution!
In general this looks good. The TODO item could be fixed right away, right?
Also (I tested this because I was not 100% sure whether it would work), the from_dhall_value()
function does not need to clone.
Here is a patch for that: matthiasbeyer@7a163a7
And here's another patch you could apply: matthiasbeyer@91dafd1
I guess you can just git pull --ff-only https://github.com/matthiasbeyer/config-rs/ pr-218
if you want to apply these 😄
Plus, I messed up the formatting (thank god we have CI for that), here's the patch that fixes it: matthiasbeyer@00ed90b You can of course still use the Sorry for messing this up... |
@matthiasbeyer Thanks for the timely and thorough review I integrated all your requested changes and comments, and then squashed them into a single commit. I also fixed the error handling case you mentioned above. Let me know if you have any other comments |
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.
Looks good.
No need to squash things into one commit, though. That only makes reviewing harder. For the next time, then... 😆
f54a5f6
to
6dd85f7
Compare
6dd85f7
to
f3a70f4
Compare
I have published a PR that should fix the problems during the build. |
Add dhall support compilation fixes.
@matthiasbeyer it looks like this PR is going stale too? The build PR referenced last in Dec 2021 was merged, and no remaining concerns with this PR for being merged? |
Yep so there's conflicts and this PR needs rebasing. |
@polarathene I'm closing in favor of your PR, since this has been sitting for almost 2 years now lol. Thanks so much for helping to push it over the finish line. |
This PR adds Dhall support using serde_dhall, a native Rust implementation of Dhall. Dhall is a functional configuration language designed to make composition of configuration files easier.
closes #123