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

Move HsYAML-based code to dhall-yaml #1514

Merged
merged 1 commit into from
Nov 7, 2019
Merged

Conversation

sjakobi
Copy link
Collaborator

@sjakobi sjakobi commented Nov 6, 2019

Fixes #1435.

Instead of creating helper packages for the shared code and tests, it seemed simpler to keep the shared code in dhall-json and move the shared tests to dhall-yaml.

TODO:

  • Move YAML tests to dhall-yaml
  • Move shared code for dhall-to-yaml[-ng] executables into dhall-json library
  • Update CI etc
  • Update documentation
  • Fix lower bound on aeson-yaml

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

@patrickmn I'm getting a test failure with the aeson-yaml-based dhallToYaml:

$ cabal test dhall-yaml
...
dhall-json
  ./tasty/data/normal
    HsYAML:     OK
    aeson-yaml: FAIL
      tasty/Main.hs:57:
      Conversion to YAML did not generate the expected output
      expected: "bool_value: true\nint_value: 1\nstring_value: \"2000-01-01\"\ntext: |\n  Plain text\nyes: y\n"
       but got: "bool_value: true\nint_value: 1\nstring_value: 2000-01-01\ntext: |\n  Plain text\nyes: y\n"
...

@patrickmn
Copy link
Sponsor Collaborator

Ok so it seems the test broke after https://github.com/dhall-lang/dhall-haskell/pull/1474/files which itself addressed that, in fact, values like 2020-05-25 do need to be quoted to prevent being converted into a UNIX timestamp by YAML parsers. Sigh, lol.

I will address that in aeson-yaml by quoting strings that only contain numbers or - (already doing that with numbers + 'e' / '.'

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

in fact, values like 2020-05-25 do need to be quoted to prevent being converted into a UNIX timestamp by YAML parsers.

So you're saying our test is wrong!? If so, we should rather change the expected test output and fix the HsYAML-based code, right?!

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

So you're saying our test is wrong!?

Ah, no. I mixed up expected and actual test output. Sorry! :/

@patrickmn
Copy link
Sponsor Collaborator

Right -- the behavior of the two libraries as configured was just the same, but still wrong. Dates do have to be quoted to avoid conversion to YAML dates.

Will push a new aeson-yaml momentarily.

@patrickmn
Copy link
Sponsor Collaborator

Ok, try with aeson-yaml 1.0.4.0

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

Thanks for the quick fix! Unfortunately I'm seeing a different test failure now:

  ./tasty/data/quoted
    HsYAML:     OK
    aeson-yaml: FAIL
      tasty/Main.hs:57:
      Conversion to YAML did not generate the expected output
      expected: "'bool_value': true\n'int_value': 1\n'string_value': '2000-01-01'\n'text': |\n  Plain text\n'yes': 'y'\n"
       but got: "'bool_value': true\n'int_value': 1\n'string_value': \"2000-01-01\"\n'text': |\n  Plain text\n'yes': 'y'\n"

@patrickmn
Copy link
Sponsor Collaborator

Uff. The problem is I single-quote "safe" strings and double-quote unsafe, and number/dates are part of the unsafes.

Unfortunately I'm quite busy today -- will need a little more time to address this.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

The problem is I single-quote "safe" strings and double-quote unsafe, and number/dates are part of the unsafes.

That actually seems like a reasonable scheme to me – we could consider adopting that for the HsYAML-based code. But I'm not really familiar with the details right now. Thoughts on this @Gabriel439?

- source .travis-functions.sh
- tar -jcvf $(mk_release_name dhall) bin/dhall
- tar -jcvf $(mk_release_name dhall-json) bin/dhall-to-json bin/dhall-to-yaml bin/json-to-dhall bin/yaml-to-dhall
- tar -jcvf $(mk_release_name dhall-json) bin/dhall-to-json bin/dhall-to-yaml bin/json-to-dhall
- tar -jcvf $(mk_release_name dhall-bash) bin/dhall-to-bash
- tar -jcvf $(mk_release_name dhall-lsp-server) bin/dhall-lsp-server
- tar -jcvf $(mk_release_name dhall-nix) bin/dhall-to-nix
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 don't understand how we're installing the binaries for dhall-to-nix, dhall-lsp-server etc. I only see the --copy-bins for dhall-json so far. Does anyone know?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed Travis fails now with:

$ tar -jcvf $(mk_release_name dhall) bin/dhall
tar: bin/dhall: Cannot stat: No such file or directory
tar: Error exit delayed from previous errors.
The command "tar -jcvf $(mk_release_name dhall) bin/dhall" exited with 1.
3.75s$ tar -jcvf $(mk_release_name dhall-json) bin/dhall-to-json bin/dhall-to-yaml bin/json-to-dhall
a bin/dhall-to-json
a bin/dhall-to-yaml
a bin/json-to-dhall
The command "tar -jcvf $(mk_release_name dhall-json) bin/dhall-to-json bin/dhall-to-yaml bin/json-to-dhall" exited with 0.
0.03s$ tar -jcvf $(mk_release_name dhall-bash) bin/dhall-to-bash
tar: bin/dhall-to-bash: Cannot stat: No such file or directory
tar: Error exit delayed from previous errors.
The command "tar -jcvf $(mk_release_name dhall-bash) bin/dhall-to-bash" exited with 1.
0.03s$ tar -jcvf $(mk_release_name dhall-lsp-server) bin/dhall-lsp-server
tar: bin/dhall-lsp-server: Cannot stat: No such file or directory
tar: Error exit delayed from previous errors.
The command "tar -jcvf $(mk_release_name dhall-lsp-server) bin/dhall-lsp-server" exited with 1.
0.03s$ tar -jcvf $(mk_release_name dhall-nix) bin/dhall-to-nix
tar: bin/dhall-to-nix: Cannot stat: No such file or directory
tar: Error exit delayed from previous errors.
The command "tar -jcvf $(mk_release_name dhall-nix) bin/dhall-to-nix" exited with 1.

Could it be that we have distributed ancient cached binaries for OS X so far?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No, it was just 96e694d that restricted the binary build to dhall-json.

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

I think we should maybe move dhall-json's Dhall.Yaml module to Dhall.JSON.Yaml or Dhall.JSON.YAML to reduce confusion with the Dhall.YAML module in dhall-yaml. Thoughts on this?

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

There's a funny build failure on Travis that I can't reproduce on Linux:

dhall-yaml> /Users/travis/build/dhall-lang/dhall-haskell/dhall-yaml/src/Dhall/YAML.hs:3:8: error:
dhall-yaml>     File name does not match module name:
dhall-yaml>     Saw: ‘Dhall.YAML’
dhall-yaml>     Expected: ‘Dhall.Yaml’
dhall-yaml>   |
dhall-yaml> 3 | module Dhall.YAML where
dhall-yaml>   |        ^^^^^^^^^^

I wonder whether that's related to the case-insensitivity of the file system or something like that?!


EDIT: Seems like a long-standing Cabal issue: haskell/cabal#4739 (comment)

@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

I think we should maybe move dhall-json's Dhall.Yaml module to Dhall.JSON.Yaml or Dhall.JSON.YAML to reduce confusion with the Dhall.YAML module in dhall-yaml. Thoughts on this?

That should also help with the build failure on OS X. I also want to go back to spelling YAML as Yaml – consistency with dhall-json's JSON is nice, but Yaml is ultimately the nicer module name component and more consistent with the pre-existing code.

@sjakobi sjakobi changed the title WIP: Move HsYAML-based code to dhall-yaml Move HsYAML-based code to dhall-yaml Nov 6, 2019
@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 6, 2019

CI still fails due to the test failure with aeson-yaml. Apart from that I think this PR ready for review! :)

dhall-yaml/release.nix Outdated Show resolved Hide resolved
* Shared code for the dhall-to-yaml[-ng] executables stays in dhall-json.
* Shared tests are in dhall-yaml.

Fixes #1435.
@sjakobi
Copy link
Collaborator Author

sjakobi commented Nov 7, 2019

I have disabled the test for now so I can merge, but made an issue to track the problem: #1516.

@mergify mergify bot merged commit cd49b65 into master Nov 7, 2019
@mergify mergify bot deleted the sjakobi/1435-split-json-yaml branch November 7, 2019 12:11
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.

Move HsYAML-based code in dhall-json to a separate package dhall-yaml
3 participants