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

Go back to using --omitNull instead of --omitEmpty #110

Merged
merged 1 commit into from
Jan 31, 2020
Merged

Conversation

Gabriella439
Copy link
Contributor

Fixes #86

The motivation of this is to more accurately model the Kubernetes
API semantics by not auto-omitting empty fields. This is because
a field set to Some ([] : List T) is not necessarily the same
as None (List T).

This makes the typical case a bit more verbose (more Somes), but
the difference to the ./examples shows that it's not too bad.

Fixes #86

The motivation of this is to more accurately model the Kubernetes
API semantics by not auto-omitting empty fields.  This is because
a field set to `Some ([] : List T)` is not necessarily the same
as `None (List T)`.

This makes the typical case a bit more verbose (more `Some`s), but
the difference to the `./examples` shows that it's not too bad.
@arianvp
Copy link
Member

arianvp commented Jan 31, 2020

Lgtm

Copy link
Collaborator

@amarrella amarrella left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this!

@PierreR
Copy link

PierreR commented May 7, 2020

The script that builds the example in dhall-kubernetes still uses --omitEmpty
https://github.com/dhall-lang/dhall-kubernetes/blob/master/scripts/build-examples.py#L49

@Gabriel439 Is it still correct or is it something that should be changed ?

@Gabriella439
Copy link
Contributor Author

@PierreR: That script is dead code and is not used. I just forgot to delete it when I replaced that logic with Nix. I have a pull request up to remove the now-defunct scripts:

#118

@PierreR
Copy link

PierreR commented May 8, 2020

@Gabriel439 Thx

Looks like the nix script also uses the --omit-empty flag: https://github.com/dhall-lang/dhall-kubernetes/blob/master/nix/nixpkgs.nix#L80 ?

As an aside question, what's the best option for dhall-kubernetes between dhall-yaml and dhall-json}/bin/dhall-to-yaml ?

@Gabriella439
Copy link
Contributor Author

@PierreR: I think both should be fine if all you use is the command-line tool. The main advantage of the dhall-yaml package is that it is based on a pure Haskell implementation of YAML instead of binding to a C library for YAML support, but it is a GPL-licensed. Functionally, they both should behave the same as far as the command-line interface goes.

I also have another pull request up to also remove the --omit-empty from the Nix code,
too: #119

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.

Go back to --omitNull instead of --omitEmpty
4 participants