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

dhall-to-{json,yaml,yaml-ng}: Handle empty maps correctly #1561

Merged
merged 1 commit into from
Dec 1, 2019

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Nov 19, 2019

While generalizing list annotations In 0ee6ce6, I had forgotten to update dhall-json.

Fixes #1559.

This requires aeson-yaml >= 1.0.5.0 since older versions
incorrectly encode empty objects.

The raised bound also fixes #1560. A regression test is included.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 19, 2019

Unfortunately dhall-to-yaml and dhall-to-yaml-ng disagree about how to encode empty objects:

  ./tasty/data/emptyMap
    HsYAML:     OK
    aeson-yaml: FAIL
      tasty/Main.hs:65:
      Conversion to YAML did not generate the expected output
      expected: "{}\n"
       but got: "\n"

Could you take a look at that @patrickmn?

@@ -32,6 +33,10 @@ testTree =
, testDhallToYaml
Dhall.JSON.Yaml.defaultOptions
"./tasty/data/special"
, testDhallToYaml
Dhall.JSON.Yaml.defaultOptions
{ conversion = Dhall.JSON.Conversion "mapKey" "mapValue" }
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wonder whether we should update these defaultOptions to accurately reflect the default CLI options.

(In testDhallToJSON, we already use this Conversion by default:

let mapKey = "mapKey"
let mapValue = "mapValue"
let conversion = Conversion {..}
let convertedExpression =
Dhall.JSON.convertToHomogeneousMaps conversion resolvedExpression

)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have prepared a patch that adds a defaultConversion, that we could apply after this fix.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it would be a good idea to keep them in sync

@patrickmn
Copy link
Sponsor Collaborator

Yep, will fix.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 26, 2019

Yep, will fix.

Ping @patrickmn! :)

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 30, 2019

@patrickmn Any chance this could make it into the new release (#1575)?

@patrickmn
Copy link
Sponsor Collaborator

Sorry, wasn't here much lately.

Yes, if it's just the empty objects and not the single vs. double-quoting right now I can push a change and release in the next hour or so.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 30, 2019

No worries @patrickmn. I would really appreciate a fix for the empty-object encoding! :)

#1516 is less important IMHO.

@patrickmn
Copy link
Sponsor Collaborator

Ok, should be fixed in 1.0.5.0 - clovyr/aeson-yaml@2b4abb1

Fixes #1559.

This requires aeson-yaml >= 1.0.5.0 since older versions
incorrectly encode empty objects.

The raised bound also fixes #1560. A regression test is included.
@sjakobi sjakobi changed the title WIP: dhall-to-{json,yaml,yaml-ng}: Handle empty maps correctly dhall-to-{json,yaml,yaml-ng}: Handle empty maps correctly Dec 1, 2019
@sjakobi
Copy link
Collaborator Author

sjakobi commented Dec 1, 2019

Thanks for the fix @patrickmn! :)

@@ -32,6 +33,10 @@ testTree =
, testDhallToYaml
Dhall.JSON.Yaml.defaultOptions
"./tasty/data/special"
, testDhallToYaml
Dhall.JSON.Yaml.defaultOptions
{ conversion = Dhall.JSON.Conversion "mapKey" "mapValue" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, it would be a good idea to keep them in sync

@mergify mergify bot merged commit 85645a2 into master Dec 1, 2019
@mergify mergify bot deleted the sjakobi/1559-empty-list branch December 1, 2019 18:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants