Skip to content
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

feat: Add Dhall support #466

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

polarathene
Copy link
Collaborator

Rebased #218 with conflicts resolved. All credit to @sphw for the actual contribution.


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

@polarathene polarathene mentioned this pull request Oct 4, 2023
@polarathene
Copy link
Collaborator Author

polarathene commented Oct 5, 2023

@matthiasbeyer since this original PR was stale from about 2 years ago, does anything look like adjustments are needed? In the tests I can see it's missing the enum settings file like all other config types seem to have coverage for. Should that be addressed?

I probably should version bump the new dependencies added too?

@polarathene

This comment was marked as outdated.

serde_dhall::SimpleValue::Num(num) => match num {
serde_dhall::NumKind::Bool(b) => Value::new(uri, ValueKind::Boolean(b)),
serde_dhall::NumKind::Natural(n) => Value::new(uri, ValueKind::Integer(n as i64)),
serde_dhall::NumKind::Integer(i) => Value::new(uri, ValueKind::Integer(i)),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While looking into version bumping serde_dhall I checked the repo and noticed it's no longer maintained.

The demand for Dhall in config-rs seems low. Perhaps this isn't worth landing into config-rs?

Cargo.toml Outdated
@@ -36,6 +37,7 @@ yaml-rust = { version = "0.4", optional = true }
rust-ini = { version = "0.19", optional = true }
ron = { version = "0.8", optional = true }
json5_rs = { version = "0.4", optional = true, package = "json5" }
serde_dhall = { version = "0.10", optional = true }
Copy link
Collaborator Author

@polarathene polarathene Oct 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
serde_dhall = { version = "0.10", optional = true }
serde_dhall = { version = "0.12", optional = true }

Note this status update from June 2023:

https://github.com/Nadrieril/dhall-rust#status

I am no longer maintaining this project.
I got it to support about 90% of the language but then lost faith in the usability of dhall for my purposes

  • Only 1 other user has 👍 the original issue (Nov 2019) requesting Dhall support: Format request: Dhall #123
  • The original PR (July 2021) itself was left stale for about 2 years, with the author no longer interested pursuing it.
  • The original issue author also expressed a loss of interest in Dhall (March 2021).
  • I personally have no interest in the config format myself. I just thought I'd help avoid a feature PR from going stale 😅

Given the above, perhaps Dhall should not be merged into config-rs? The lack of upstream maintenance and demand doesn't seem to justify any additional maintenance burden it may offload onto config-rs project?

Comment on lines 14 to 25
let value = from_dhall_value(uri, serde_dhall::from_str(text).parse()?);
match value.kind {
ValueKind::Table(map) => Ok(map),
ValueKind::Nil => Err(Unexpected::Unit),
ValueKind::Boolean(value) => Err(Unexpected::Bool(value)),
ValueKind::Integer(value) => Err(Unexpected::Integer(value)),
ValueKind::Float(value) => Err(Unexpected::Float(value)),
ValueKind::String(value) => Err(Unexpected::Str(value)),
ValueKind::Array(value) => Err(Unexpected::Seq),
}
.map_err(|err| ConfigError::invalid_root(uri, err))
.map_err(|err| Box::new(err) as Box<dyn Error + Send + Sync>)

This comment was marked as resolved.

sphw and others added 2 commits October 6, 2023 11:23
Signed-off-by: Brennan Kinney <[email protected]>
- fix: `HashMap` =>  `Map` to avoid compilation error
  Original author: MGlolenstine
- fix: `Integer` => `I64`
  `ValueKind` enum has changed since the original PR went stale.
- chore: Version bump `serde_dhall` to `0.12`

Signed-off-by: Brennan Kinney <[email protected]>
@matthiasbeyer
Copy link
Member

The demand for Dhall in config-rs seems low. Perhaps this isn't worth landing into config-rs?

Yes, depending on an unmaintained implementation is definitively something we shouldn't rely on, sorry.

@polarathene
Copy link
Collaborator Author

FWIW, it seems that the yaml support has become unmaintained, no commits since July 2021. The developer doesn't have much activity on Github (last active Sep 2022).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Format request: Dhall
4 participants