Skip to content

Conversation

patrickmn
Copy link
Collaborator

Fixes #1516: '2020-02-27' is now single-quoted by both aeson-yaml and HsYAML.

Fixes #1617: "hello world" is no longer quoted.

HsYAML was single-quoting when quoted was True and double-quoting otherwise. Changed its behavior so it will single-quote strings containing dates and bools in both cases.

@patrickmn
Copy link
Collaborator Author

patrickmn commented Feb 27, 2020

@sjakobi, I think the "right" long-term solution is to use HsYAML's default (fall back to double quoting) both when quoting is True and False, and then change aeson-yaml to remove the places where we single-quote strings to be compatible with the current configuration. Is there a reason not to do that?

@patrickmn patrickmn requested a review from sjakobi February 27, 2020 21:12
Copy link
Collaborator

@sjakobi sjakobi left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a good step forward to me!

I actually don't know why we went with single-quotes in the case of --quoted. Perhaps for compatibility with the old yaml-based code?! Any idea @Gabriel439?

@Gabriella439
Copy link
Collaborator

@sjakobi: I was just preserving the existing behavior. All I know is that single quotes don't interpret escape codes, whereas double quotes do

@Gabriella439
Copy link
Collaborator

@patrickmn: You can fix the "hydra" build failure by running:

$ cabal update
$ cabal2nix cabal://aeson-yaml-1.0.6 > ./nix/aeson-yaml.nix

@patrickmn
Copy link
Collaborator Author

Ok, I guess let's not fix it if it isn't broken (*cough*YAML*cough*). We can consider a change if there are any more discrepancies.

@sjakobi
Copy link
Collaborator

sjakobi commented Feb 28, 2020

Ok, I guess let's not fix it if it isn't broken (coughYAMLcough). We can consider a change if there are any more discrepancies.

Are you referring to your plan above for the "right" solution (with more double-quoting IIUC)?

Actually I didn't quite get the motivation that plan. Is it to avoid the complex style decision in jsonToYaml?

In general I'm all for finding a sweet spot with consistent and as-simple-as-possible-but-no-simpler encoding rules! But I don't have good grasp of the intricacies involved right now.

@Gabriella439
Copy link
Collaborator

@patrickmn: The "hydra" build is failing on this warning:

tasty/Main.hs:17:1: error: [-Wunused-imports, -Werror=unused-imports]
    The qualified import of ‘Test.Tasty.ExpectedFailure’ is redundant
      except perhaps to import instances from ‘Test.Tasty.ExpectedFailure’
    To import instances alone, use: import Test.Tasty.ExpectedFailure()
   |
17 | import qualified Test.Tasty.ExpectedFailure
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@patrickmn
Copy link
Collaborator Author

Are you referring to your plan above for the "right" solution (with more double-quoting IIUC)?

Yeah, I mean let's merge this, and if inconsistencies continue to be a headache, we can discuss whether a change like only using double-quoting makes sense.

Actually I didn't quite get the motivation that plan. Is it to avoid the complex style decision in jsonToYaml?

And special-casing in aeson-yaml to single-quote instead of double-quote.

In general I'm all for finding a sweet spot with consistent and as-simple-as-possible-but-no-simpler encoding rules! But I don't have good grasp of the intricacies involved right now.

Same :)

@patrickmn patrickmn merged commit 7005842 into dhall-lang:master Feb 29, 2020
@sjakobi
Copy link
Collaborator

sjakobi commented Feb 29, 2020

@patrickmn Would you please allow a bit more time for discussion and wait for an approval from @Gabriel439 or me the next time? (Unless Gabriel gave you permission to merge without approval, which I may have missed.)

In this case I'm a bit concerned about the churn in the YAML output formatting, and was wondering whether we should delay this PR until we have properly considered the double-quoting solution you mentioned. No biggie though, and no need to revert IMHO.

@patrickmn
Copy link
Collaborator Author

patrickmn commented Feb 29, 2020

Sure, my apologies. I thought you had approved this change and was merely asking questions about the long-term suggestion rationale. (I'm also not sure it's worth it to switch to double quotes everywhere, it just simplifies both libraries.)

Happy to do another PR based on the continued discussion as well.

@sjakobi
Copy link
Collaborator

sjakobi commented Feb 29, 2020

No worries! I'm aware that I sometimes don't communicate very clearly, but I do signal explicit approval via GitHub's "Approve" button.

I guess I've been a bit frustrated about the amount of churn and inconsistencies that the move away from yaml entailed. At this point I'm pretty optimistic that the worst is over though! ;)

…and I guess I should remind myself sometimes that it's ok when things are just good enough! ;)

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.

yaml strings are quoted unnecessarily dhall-to-yaml and dhall-to-yaml-ng quote inconsistently
3 participants