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

dhall-json: Add a defaultConversion for converting maps #1579

Merged
merged 2 commits into from
Dec 2, 2019

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Dec 2, 2019

No description provided.

@@ -45,7 +46,7 @@ defaultOptions =
, omission = id
, documents = False
, quoted = False
, conversion = NoConversion
, conversion = Dhall.JSON.defaultConversion
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe there are more aspects where these defaultOptions diverge from the default behaviour of dhall-yaml[-ng], for example omission. I think it would be nice if these defaultOptions would exactly match the CLI defaults, and were in fact used by the executables.

I wonder whether we should also try to share these Options between dhall-json and dhall-yaml[-ng]

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think this is fine. My understanding is that dhall-yaml already has a dependency on dhall-json anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right. dhall-to-json currently has it's own Options record:

data Options
= Options
{ explain :: Bool
, pretty :: Bool
, omission :: Value -> Value
, conversion :: Conversion
, approximateSpecialDoubles :: Bool
, file :: Maybe FilePath
, output :: Maybe FilePath
}
| Version

The fields that it shares with dhall-to-yaml[-ng] could in principle live in a common DhallToXOptions record or something like that.

@mergify mergify bot merged commit de51daf into master Dec 2, 2019
@mergify mergify bot deleted the sjakobi/default-conversion branch December 2, 2019 23:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants