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.

Sign up for GitHub

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

Make some of CLI options for dhall-json more consistent #1475

Merged
merged 4 commits into from
Oct 29, 2019
Merged

Make some of CLI options for dhall-json more consistent #1475

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6081204
Make some of CLI options for dhall-json more consistent
astynax Oct 25, 2019
2cd457d
fix option parser and array nullification
astynax Oct 28, 2019
541a451
update changelog
astynax Oct 28, 2019
463c904
Merge branch 'master' into dhall-json-cli-options-renaming
mergify[bot] Oct 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions dhall-json/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
Next version

* [BREAKING CHANGE: Rename some options of `dhall-to-{json,yaml}` to more consistent ones](https://github.com/dhall-lang/dhall-haskell/issues/1430):
* rename `--omitEmpty` to `--omit-empty`
* rename `--preserveNull` to `--preserve-null`
* rename `--noMaps` to `--no-maps`
Copy link
Collaborator

Choose a reason for hiding this comment

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

The changes below have already been released. Can you separate the new entries?

* drop `--omitNull` as redundant because of `--preserve-null` (see below)
* [BREAKING CHANGE: Enable `--pretty` by default for `dhall-to-json`](https://github.com/dhall-lang/dhall-haskell/issues/716)
* [BREAKING CHANGE: Enable `--omitNull` by default for `dhall-to-{json,yaml}`](https://github.com/dhall-lang/dhall-haskell/pull/1365)
* To recover the old behavior use the `--preserveNull` flag
Expand Down
17 changes: 5 additions & 12 deletions dhall-json/src/Dhall/JSON.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,6 @@ module Dhall.JSON (
dhallToJSON
, omitNull
, omitEmpty
, parseOmission
, parsePreservationAndOmission
, Conversion(..)
, convertToHomogeneousMaps
Expand Down Expand Up @@ -609,7 +608,7 @@ omitEmpty (Object object) =
omitEmpty (Array array) =
if null elems then Null else Array elems
where
elems = (fmap omitEmpty array)
elems = Vector.filter (/= Null) (fmap omitEmpty array)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There was a bug

omitEmpty (String string) =
String string
omitEmpty (Number number) =
Expand All @@ -623,30 +622,24 @@ omitEmpty Null =
parseOmission :: Parser (Value -> Value)
parseOmission =
Options.Applicative.flag'
omitNull
( Options.Applicative.long "omitNull"
<> Options.Applicative.help "Omit record fields that are null"
)
<|> Options.Applicative.flag'
omitEmpty
( Options.Applicative.long "omitEmpty"
( Options.Applicative.long "omit-empty"
Copy link
Collaborator

Choose a reason for hiding this comment

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

What about the move from --omitEmpty to --omissible-lists discussed in #1414 and originally proposed in #1410?

Would you mind checking that --omitEmpty and --omissible-lists are indeed opposites?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

omit-mpty also "nullifies" not only empty arrays but also empty objects. omissible-lists translates only null ~> [] but not null ~> {}. So omit-empty and omissible-lists are not complete opposites.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for checking! Seems like a bit of an unfortunate oversight! Let's maybe keep the --omit-empty name for now then, and discuss this further on the issue.

<> Options.Applicative.help "Omit record fields that are null or empty records"
)
<|> pure id

-- | Parser for command-line options related to preserving null fields.
parseNullPreservation :: Parser (Value -> Value)
parseNullPreservation =
Options.Applicative.flag
omitNull
id
( Options.Applicative.long "preserveNull"
( Options.Applicative.long "preserve-null"
<> Options.Applicative.help "Preserve record fields that are null"
)

-- | Combines parsers for command-line options related to preserving & omitting null fields.
parsePreservationAndOmission :: Parser (Value -> Value)
parsePreservationAndOmission = parseNullPreservation <|> parseOmission <|> pure id
parsePreservationAndOmission = parseOmission <|> parseNullPreservation

{-| Specify whether or not to convert association lists of type
@List { mapKey: Text, mapValue : v }@ to records
Expand Down Expand Up @@ -1038,7 +1031,7 @@ parseConversion =
noConversion =
Options.Applicative.flag'
NoConversion
( Options.Applicative.long "noMaps"
( Options.Applicative.long "no-maps"
<> Options.Applicative.help "Disable conversion of association lists to homogeneous maps"
)

Expand Down