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

Optionally allow missing lists in yaml/json-to-dhall #1414

Merged
merged 2 commits into from
Oct 11, 2019

Conversation

akshaymankar
Copy link
Contributor

Fixes #1410

@sjakobi
Copy link
Collaborator

sjakobi commented Oct 11, 2019

This looks great! My understanding of #1410 was that we also want the --omissible-lists option to replace --omitEmpty in dhall-to-json and dhall-to-yaml. Would you like to implement this too? Otherwise this could be done in a separate patch.

@akshaymankar
Copy link
Contributor Author

Sure I can do that also in this PR. As this would be a breaking change, would it be better to have deprecation warnings for omitEmpty?

Also, should I rename --omitNull to --omit-null (omissible-null sounds weird) and --noMaps to --no-maps for consistency of case?

@sjakobi
Copy link
Collaborator

sjakobi commented Oct 11, 2019

As this would be a breaking change, would it be better to have deprecation warnings for omitEmpty?

It might be a good idea to keep --omitEmpty around for a bit. AFAIK we haven't used actual deprecation warnings so far, so I don't think it's necessary to add some in this case.

Also, should I rename --omitNull to --omit-null (omissible-null sounds weird)

--omitNull is the default setting now, so I think we can simply get rid of it soon.

and --noMaps to --no-maps for consistency of case?

Yes, let's get rid of camelCase in CLI options. Not sure whether we need to keep --noMaps around again… Thoughts @Gabriel439?

The new --preserveNull option should also be changed to --preserve-null.

@sjakobi
Copy link
Collaborator

sjakobi commented Oct 11, 2019

Seems like the updating of the CLI options would be a well-sized change on its own, so let's maybe merge this as-is and change the options in a follow-up PR. Would that be ok for you, @akshaymankar?

@akshaymankar
Copy link
Contributor Author

Yeah that would be ok with me, feel free to merge :)

@mergify mergify bot merged commit 194654c into dhall-lang:master Oct 11, 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.

[Feature Request] Tolerate missing lists and maps in yaml-to-dhall and json-to-dhall
2 participants