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

Add the --preserveNull flag and omit null fields by default on dhall-to-yaml #1365

Merged
merged 6 commits into from
Oct 4, 2019

Conversation

mujx
Copy link
Contributor

@mujx mujx commented Oct 1, 2019

Currently this is a breaking change because the --omitNull flag is removed, but it would seem a bit confusing to keep it and have two flags for the same purpose. Let me know if I should keep it.

$ dhall-to-yaml --preserveNull <<< '{ a = "dhall", b = None Natural, c = [ ] : List Text }'
a: dhall
b: null
c: []

$ dhall-to-yaml <<< '{ a = "dhall", b = None Natural, c = [ ] : List Text }'
a: dhall
c: []

$ dhall-to-yaml --omitEmpty <<< '{ a = "dhall", b = None Natural, c = [ ] : List Text }'
a: dhall

@sjakobi
Copy link
Collaborator

sjakobi commented Oct 1, 2019

Many thanks for the PR! :)

Currently this is a breaking change because the --omitNull flag is removed

I'm confused. The code for parsing that option is still there.

IMHO it's OK to keep --omitNull for a bit, for backwards compatibility, but we should probably add a TODO as a reminder to eventually remove it.

Can you also update this example?

> $ cat ./example.dhall
> let JSON = https://prelude.dhall-lang.org/JSON/package.dhall
>
> in JSON.object
> [ { mapKey = "foo", mapValue = JSON.null }
> , { mapKey =
> "bar"
> , mapValue =
> JSON.array [ JSON.number 1.0, JSON.bool True ]
> }
> ]
> $ dhall-to-json <<< './example.dhall'
> {"foo":null,"bar":[1,true]}

@mujx
Copy link
Contributor Author

mujx commented Oct 1, 2019

The code is still there because it's still used by dhall-to-json which continues to have both --omitNull and --omitEmpty.

Unless I should also update dhall-to-json.

@sjakobi
Copy link
Collaborator

sjakobi commented Oct 1, 2019

Ah, that makes sense! 👍

Sorry for the confusion!

@Gabriella439
Copy link
Collaborator

I think it's worth keeping the --omitNull flag (even as a no-op) for at least one release to allow for a smooth migration path

Also, even thought I only requested this for dhall-to-yaml this discussion makes me realize that we should probably do the same for dhall-to-json, too

dhall-json/src/Dhall/JSON.hs Outdated Show resolved Hide resolved
dhall-json/src/Dhall/JSON.hs Show resolved Hide resolved
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

LGTM, apart from one more wibble. 👍 :)

dhall-json/src/Dhall/JSON.hs Outdated Show resolved Hide resolved
@sjakobi
Copy link
Collaborator

sjakobi commented Oct 3, 2019

@Gabriel439 I think this is fine to merge. Could you give it a quick look?

@Gabriella439
Copy link
Collaborator

Yeah, this looks great to me! I just added the merge-me label

@sjakobi: For future reference, you can merge without my approval. If I see anything that still needs to be fixed I can always create a follow-up pull request

@mergify mergify bot merged commit 045ed99 into dhall-lang:master Oct 4, 2019
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

3 participants