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

astynax · 2019-10-25T07:59:51Z

sjakobi

Regarding the renaming of the --preserveNull and --noMaps options, I think the breaking change is ok: --preserveNull is brand new, and I suspect that --noMaps is not very popular. Thoughts @Gabriel439?

sjakobi · 2019-10-25T10:35:27Z

dhall-json/src/Dhall/JSON.hs

@@ -624,12 +624,12 @@ parseOmission :: Parser (Value -> Value)
 parseOmission =
        Options.Applicative.flag'
            omitNull
-            (   Options.Applicative.long "omitNull"
+            (   Options.Applicative.long "omit-null"


I'm not sure whether it's worth breaking compatibility here. --omitNull is the default setting, and I was assuming that we could get rid of it soon.

--omitNull is the default setting

I have a question about it. This parser looks like one that always return omitNull because of flag (that always succeed) as a first Alternative.

Do you mean this bit?

dhall-haskell/dhall-json/src/Dhall/JSON.hs

Lines 639 to 642 in 90315e9

parseNullPreservation =

Options.Applicative.flag

omitNull

id

omitNull is the default value here, so if we pass --preserveNull, the result is id.

http://hackage.haskell.org/package/optparse-applicative-0.15.1.0/docs/Options-Applicative-Builder.html#v:flag

This parser returns a function in any case. It can't fail. So

parseNullPreservation <|> smth

will never try to use smth.

This means that

parsePreservationAndOmission = parseNullPreservation <|> parseOmission <|> pure id

will never use parseOmissions because of always successful parseNullPreservation.

Oh, yes, you're completely right! Good catch! :)

Can you fix this so --omitNull and --omitEmpty still have an effect when passed?

As I can see, for now omit-null is complete useless: the omitNull is a default value for preserve-null flag. So this two flags are opposites.

If default behavior is omitting of nulls, I propose to remove --omit-null as redundant.

Just removed --omit-nulls.

sjakobi · 2019-10-25T10:41:04Z

dhall-json/src/Dhall/JSON.hs

            <>  Options.Applicative.help "Omit record fields that are null"
            )
    <|> Options.Applicative.flag'
            omitEmpty
-            (   Options.Applicative.long "omitEmpty"
+            (   Options.Applicative.long "omit-empty"


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?

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.

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.

Gabriella439 · 2019-10-25T15:35:25Z

@sjakobi: Yeah, I'm pretty sure nobody will complain if we rename those

astynax · 2019-10-28T08:30:22Z

dhall-json/src/Dhall/JSON.hs

@@ -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)


There was a bug

sjakobi · 2019-10-28T14:15:56Z

dhall-json/src/Dhall/JSON.hs

            <>  Options.Applicative.help "Omit record fields that are null"
            )
    <|> Options.Applicative.flag'
            omitEmpty
-            (   Options.Applicative.long "omitEmpty"
+            (   Options.Applicative.long "omit-empty"


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.

sjakobi · 2019-10-28T14:17:46Z

dhall-json/CHANGELOG.md

@@ -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 `--omitNull` to `--omit-null`


Can you fix this line?

sjakobi · 2019-10-28T14:17:56Z

dhall-json/CHANGELOG.md

@@ -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 `--omitNull` to `--omit-null`
+    * rename `--omit-empty` to `--omit-empty`


Suggested change

* rename `--omit-empty` to `--omit-empty`

* rename `--omitEmpty` to `--omit-empty`

sjakobi · 2019-10-28T14:19:22Z

dhall-json/CHANGELOG.md

+    * rename `--omitNull` to `--omit-null`
+    * rename `--omit-empty` to `--omit-empty`
+    * rename `--preserveNull` to `--preserve-null`
+    * rename `--noMaps` to `--no-maps`


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

sjakobi

LGTM! :)

sjakobi reviewed Oct 25, 2019

View reviewed changes

astynax commented Oct 28, 2019

View reviewed changes

sjakobi reviewed Oct 28, 2019

View reviewed changes

astynax added 3 commits October 28, 2019 22:14

Make some of CLI options for dhall-json more consistent

6081204

fix option parser and array nullification

2cd457d

update changelog

541a451

sjakobi approved these changes Oct 28, 2019

View reviewed changes

sjakobi requested a review from Gabriella439 October 28, 2019 20:06

Gabriella439 approved these changes Oct 29, 2019

View reviewed changes

sjakobi added the merge me label Oct 29, 2019

Merge branch 'master' into dhall-json-cli-options-renaming

463c904

mergify bot merged commit 2a8735c into dhall-lang:master Oct 29, 2019

astynax deleted the dhall-json-cli-options-renaming branch October 30, 2019 06:09

sjakobi mentioned this pull request Oct 30, 2019

Asymmetry between --omit-empty and --omissible-lists #1488

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

astynax commented Oct 25, 2019

sjakobi left a comment

sjakobi Oct 25, 2019

astynax Oct 25, 2019

sjakobi Oct 25, 2019

astynax Oct 25, 2019

astynax Oct 25, 2019 •

edited

Loading

sjakobi Oct 25, 2019

astynax Oct 25, 2019

astynax Oct 28, 2019

astynax Oct 28, 2019

astynax Oct 28, 2019

sjakobi Oct 25, 2019

astynax Oct 28, 2019

sjakobi Oct 28, 2019

Gabriella439 commented Oct 25, 2019

astynax Oct 28, 2019

sjakobi Oct 28, 2019

sjakobi Oct 28, 2019

astynax Oct 28, 2019

sjakobi Oct 28, 2019

astynax Oct 28, 2019

sjakobi Oct 28, 2019

sjakobi left a comment

	* rename `--omit-empty` to `--omit-empty`
	* rename `--omitEmpty` to `--omit-empty`

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

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

Conversation

astynax commented Oct 25, 2019

sjakobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astynax Oct 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gabriella439 commented Oct 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjakobi left a comment

Choose a reason for hiding this comment

astynax Oct 25, 2019 •

edited

Loading