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 a `Prelude.JSON.omitNullFields` #784

Merged
merged 5 commits into from Oct 29, 2019
Merged

Add a `Prelude.JSON.omitNullFields` #784

merged 5 commits into from Oct 29, 2019

Conversation

@Gabriel439
Copy link
Contributor

Gabriel439 commented Oct 21, 2019

This is one step towards moving more dhall-to-json functionality into pure
Dhall. This implements the --omitNull functionality of the
command-line tool.

This is one step towards moving more `dhall-to-json` functionality into pure
Dhall.  This implements the `--omitNull` functionality of the
command-line tool.
@Nadrieril

This comment has been minimized.

Copy link
Collaborator

Nadrieril commented Oct 21, 2019

This doesn't seem to filter [1, null, 2] into [1, 2]; is this expected ?

(Also, you forgot to update the preludeB test)

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Oct 21, 2019

This is one step towards moving more dhall-to-json functionality into pure
Dhall. This implements the --omitNull functionality of the
command-line tool.

But dhall-to-json can't handle Prelude.JSON.Type as an input, right?

@Gabriel439

This comment has been minimized.

Copy link
Contributor Author

Gabriel439 commented Oct 21, 2019

@sjakobi: Actually, dhall-to-json can handle Prelude.JSON.Type:

-- ./example.dhall
let JSON = https://prelude.dhall-lang.org/JSON/package.dhall

in  JSON.array [JSON.number 1.0, JSON.bool True ]
$ dhall-to-json --file ./example.dhall
[
  1,
  true
]

... although that's not what I meant. What I mean is that given a Dhall value of type A, a package author could implement a pure Dhall function to convert As to JSON and then use this utility to filter out nulls and render that to JSON using the render function

@Gabriel439

This comment has been minimized.

Copy link
Contributor Author

Gabriel439 commented Oct 21, 2019

@Nadrieril: Yes, that is intentional. I can add another test along those lines

... based on discussoin with @Nadrieril
@f-f
f-f approved these changes Oct 22, 2019
... as caught by @Nadrieril
@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Oct 24, 2019

I think the name "filterNullFields" is slightly inconsistent with List.filter. In List.filter, the predicate determines the elements that we keep. Here, it's the null fields that we remove. Something like removeNullFields would be a bit better IMHO.

OTOH, a function that keeps only the null fields would be pretty useless, so most users can guess that filterNullFields filters the null fields out

@Gabriel439

This comment has been minimized.

Copy link
Contributor Author

Gabriel439 commented Oct 24, 2019

@sjakobi: Another idea I had was that we could call it omitNull, for consistency with the dhall-to-{json,yaml} commands

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Oct 24, 2019

Another idea I had was that we could call it omitNull, for consistency with the dhall-to-{json,yaml} commands

That seems like a fine name! I expect that since that the --omitNull is the default now, we'll get rid of it at some point though.

@Gabriel439

This comment has been minimized.

Copy link
Contributor Author

Gabriel439 commented Oct 25, 2019

@sjakobi: Actually, I'll probably go with omitNullFields, just so that the name clarifies that it does not omit list elements

... based on discussion with @sjakobi
@Gabriel439 Gabriel439 changed the title Add a `Prelude.JSON.filterNullFields` Add a `Prelude.JSON.omitNullFields` Oct 25, 2019
@Gabriel439 Gabriel439 merged commit 5c89715 into master Oct 29, 2019
1 check passed
1 check passed
hydra Hydra build #42140 of dhall-lang:784:dhall-lang
Details
@Gabriel439 Gabriel439 deleted the gabriel/filterNullFields branch Oct 29, 2019
Gabriel439 added a commit that referenced this pull request Oct 29, 2019
In #784 I renamed the
`filterNullFields` function to `omitNullFields` without a corresponding change
to the Prelude expected output, which this change fixes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.