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

Implement renderYAML #799

Merged
merged 13 commits into from Nov 7, 2019
Merged

Implement renderYAML #799

merged 13 commits into from Nov 7, 2019

Conversation

@matheus23
Copy link
Collaborator

matheus23 commented Oct 30, 2019

Hi! This is a first attempt at https://discourse.dhall-lang.org/t/expense-proposal-pure-dhall-function-to-render-yaml/92.

I've wanted to get started with Dhall some time ago and this seemed like a nice first project.

As this was my first Dhall project, there might be some things that I've gotten completely wrong, that's why I'm reporting my progress early. This is only a prototype and I'd love some review.

There are some things this is doing differently than libyaml. It seems libyaml removes duplicate fields in objects, my prototype doesn't do that. Also, Double literals are rendered as 1.0 instead of 1.

I haven't tested many edge cases, as I don't know how to get some real-world test data, so I'd love support in that direction!

@matheus23

This comment has been minimized.

Copy link
Collaborator Author

matheus23 commented Oct 30, 2019

Here is a test file I used to test this with:

let JSON = ./Prelude/JSON/package.dhall

let renderYAML = ./Prelude/JSON/renderYAML

let renderedYAML =
      renderYAML
        ( JSON.array
            [ JSON.bool True
            , JSON.string "Hello"
            , JSON.object
                [ { mapKey = "foo", mapValue = JSON.null }
                , { mapKey = "bar", mapValue = JSON.number 1.0 }
                , { mapKey = "object"
                  , mapValue =
                      JSON.object
                        [ { mapKey = "empty"
                          , mapValue =
                              JSON.object
                                ( [] : List
                                         { mapKey : Text, mapValue : JSON.Type }
                                )
                          }
                        , { mapKey = "empty"
                          , mapValue = JSON.array ([] : List JSON.Type)
                          }
                        ]
                  }
                ]
            , JSON.array [ JSON.bool True, JSON.bool False ]
            ]
        )

in  ''
    # Is this YAML yet?
    ${renderedYAML}
    ''
$ dhall --file test.dhall
''
Is this YAML yet?
- true
- "Hello"
- foo: null
  bar: 1.0
  object:
    empty: {}
    empty: []
- - true
  - false
''
@matheus23

This comment has been minimized.

Copy link
Collaborator Author

matheus23 commented Oct 31, 2019

I have been taking a closer look at YAML escaping rules.

I wanted to write up a small post about what I found out, but it turns out, its really complicated, and someone else already wrote a lot about it.

In the worst case (multiline strings with trailing whitespace), libyaml simply resorts to using single-line double-quotation as an escaping mechanism:

key: value
? "key with multiline: string \"\nand - special characters "
: null

(despite github's yaml syntax highlighting messing this up, this is libyaml-generated)

For everything in between, there are many ways to handle it: Various multiline formats that preserve or don't preserve whitespace and single-quoted strings with an exclamation mark.

I'm unsure how to proceed.
Using Text/show it would be possible to generate (probably) always valid, but very ugly YAML.

If I had some primitive for checking for the existence of certain properties of a given Text, like leading/trailing whitespace or the ": " or "- " characters for example, then I could only conditionally YAML-escape keys and values.

What to do now depends heavily on the usecase:

  • Do such strings that need to be escaped even come up in the real world? Does it make sense to handle these cases if these edge cases practically would never come up?
  • Or do these edge cases come up but nobody looks at the generated YAML files anyway, so whether they're pretty doesn't matter?
  • Or do these edge cases ocassionally come up and we should try to have some minimum effort (double-quoted escaping) for supporting them?
@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Oct 31, 2019

I think we should just go with Text/show for rendering strings. Without the ability of checking for certain substrings, I think there's nothing else that we can do and that would still work in the general case.

If I had some primitive for checking for the existence of certain properties of a given Text, like leading/trailing whitespace or the ": " or "- " characters for example, then I could only conditionally YAML-escape keys and values.

AFAIK there are no plans for adding such primitives since they would be difficult to standardize.

Copy link
Contributor

Gabriel439 left a comment

Excellent job! This is exactly what I had in mind 🙂

I only have a few cosmetic suggestions

Also, don't forget to reference this in ./Prelude/JSON/package.dhall

Prelude/JSON/renderYAML Outdated Show resolved Hide resolved
Prelude/JSON/renderYAML Outdated Show resolved Hide resolved
Prelude/JSON/renderYAML Outdated Show resolved Hide resolved
Prelude/JSON/renderYAML Show resolved Hide resolved
Prelude/JSON/renderYAML Outdated Show resolved Hide resolved
matheus23 added 6 commits Nov 1, 2019
Refine 'object' case to look similar to 'array' case in renderYAML
dhall freeze --cache --all --inplace renderYAML
@matheus23

This comment has been minimized.

Copy link
Collaborator Author

matheus23 commented Nov 1, 2019

I factored out a NonEmpty type with uncons, nonEmptyToList and concatNonEmpty functions. It might be useful to add those to the Prelude at some point, right?

Also, I'm having a hard time understanding what's wrong in the CI. I added integrity checks to the imports and added renderYAML to the package.dhall. What else do I have to do?

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Nov 1, 2019

I factored out a NonEmpty type with uncons, nonEmptyToList and concatNonEmpty functions. It might be useful to add those to the Prelude at some point, right?

I would welcome such an addition to the Prelude! :)

Also, I'm having a hard time understanding what's wrong in the CI. I added integrity checks to the imports and added renderYAML to the package.dhall. What else do I have to do?

I think you need freeze the local imports too. Try dhall freeze --cache --all.

And Prelude/package.dhall needs to be updated with the new hash for the JSON package.dhall.

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Nov 1, 2019

Some feedback: It was weird to be both looking at the wiki cheatsheet page and the Prelude directory for functions that do what I want. The cheatsheet seems to contain some functions from the Prelude, but not all, which was confusing. (Maybe it is just outdated? In some places it still uses the old [ x ] : Optional x syntax).

The cheatsheet documents only the built-in functions – I think it would get too large if we'd document the entire Prelude there. We do need better documentation for the Prelude too though! (See #760.)

We should definitely fix the outdated syntax though!

@Gabriel439

This comment has been minimized.

Copy link
Contributor

Gabriel439 commented Nov 1, 2019

@matheus23: Yeah, there is currently no documentation for the Prelude (other than the documentation within the files). Dhall does not currently have a documentation generator analogous to Haskell's haddock

If you have Nix installed you can satisfy CI by running the ./scripts/lint-prelude.sh script.

Even if you don't have Nix installed, you can still see what you need to do to fix the files here:

dhall-lang/release.nix

Lines 78 to 87 in da02ab2

expected-prelude = pkgsNew.runCommand "expected-prelude" {} ''
${pkgsNew.rsync}/bin/rsync --archive ${./Prelude}/ "$out"
${pkgsNew.coreutils}/bin/chmod --recursive u+w "$out"
for FILE in $(${pkgsNew.findutils}/bin/find "$out" -type f ! -name README.md); do
${pkgsNew.dhall}/bin/dhall lint --inplace "$FILE"
XDG_CACHE_HOME=/var/empty ${pkgsNew.dhall}/bin/dhall freeze --all --cache --inplace "$FILE"
done
'';

I also just fixed the outdated Optional syntax in the Cheatsheet

Copy link
Contributor

Gabriel439 left a comment

Looks great!

Prelude/JSON/renderYAML Outdated Show resolved Hide resolved
Prelude/JSON/renderYAML Outdated Show resolved Hide resolved
Trying to keep everything consistent
@matheus23

This comment has been minimized.

Copy link
Collaborator Author

matheus23 commented Nov 4, 2019

I now introduced a Prelude/JSON/build file that includes all types and helper functions for creating JSON values.

I changed omitNullFields and JSON.render so that they now import ./build so it is consistent between renderYAML and the other modules (and extracted the example from JSON.render out to an assertion, while I was at it).

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Nov 5, 2019

I like the structure that the build file offers, but I don't really like the name: It looks just like the other exports like bool or null – although AFAICT it's not intended for public consumption!

Can we rename it to something like internal.dhall (with the extension!) to make it clearer that it's not just another function, and users can simply ignore it?

A comment at the top of the file should explain why the file exists.

@Gabriel439

This comment has been minimized.

Copy link
Contributor

Gabriel439 commented Nov 5, 2019

@sjakobi: Note that it's not truly internal because users can still import it. Maybe build is not the right name, but I think it should still have a descriptive name rather than internal

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Nov 5, 2019

Note that it's not truly internal because users can still import it. Maybe build is not the right name, but I think it should still have a descriptive name rather than internal.

Then core.dhall? constructors.dhall? avoid-import-cycle.dhall?

@Gabriel439

This comment has been minimized.

Copy link
Contributor

Gabriel439 commented Nov 5, 2019

@sjakobi: core.dhall seems fine to me

@matheus23

This comment has been minimized.

Copy link
Collaborator Author

matheus23 commented Nov 5, 2019

I agree about ./build actually. It's also not consistent with List/build or Optional/build etc.

I also like core.dhall most. constructor.dhall wouldn't cover Type being in this module.

matheus23 added 2 commits Nov 5, 2019
JSON/build is unlike List/build or Optional/build and more similar to a package.dhall file.
@@ -0,0 +1,44 @@
{- A record of functions useful for constructing `JSON` values.

This comment has been minimized.

Copy link
@sjakobi

sjakobi Nov 5, 2019

Collaborator

This line reads as if Prelude users ought to use this record.

This comment has been minimized.

Copy link
@Gabriel439

Gabriel439 Nov 5, 2019

Contributor

Actually, maybe they ought to! It's sort of a historical accident that these core utilities are in the same record as other utilites (like ./Nesting). If we weren't worried about backwards compatibility I would have reorganized the ./JSON subdirectory a bit

This comment has been minimized.

Copy link
@sjakobi

sjakobi Nov 5, 2019

Collaborator

Right! I had noticed the inconsistency in this directory too. How would you organize it?

This comment has been minimized.

Copy link
@matheus23

matheus23 Nov 6, 2019

Author Collaborator

I was hoping the sentence after this one would clarify this.

Maybe use used instead of useful?

I like to have a sentence about what something is in that something's documentation, instead of documentation saying you shouldn't use this. (Documentation saying 'don't use this' doesn't seem like documentation for me).

I know that this module shouldn't be used by Prelude users, but I don't think documentation should be for prelude users only. Contributors might also wonder what the reason for this file is.

EDIT: For some reason I didn't see the new comments on this yet

This comment has been minimized.

Copy link
@sjakobi

sjakobi Nov 6, 2019

Collaborator

I guess my intuition is that the documentation of the Prelude should be optimized for Prelude users. So I think a good first sentence would be

Suggested change
{- A record of functions useful for constructing `JSON` values.
{- Functions for constructing `JSON` values for internal usage

The "internal" should signal to users that this file is probably not interesting to them, so they can quickly move on.

But I realize that this is premature optimization. Sorry for being so nitpicky and thanks for having addressed my concerns so far! :)

@matheus23

This comment has been minimized.

Copy link
Collaborator Author

matheus23 commented Nov 6, 2019

I'm having trouble making the CI happy again :/ Locally, I'm running a bash script that I extracted out of the release.nix file to attempt to do something similar to CI:

#!/bin/bash
for FILE in $(find "Prelude" -type f ! -name README.md); do
    echo "fixing file: $FILE"
    dhall lint --inplace "$FILE"
    dhall format --inplace "$FILE"
    XDG_CACHE_HOME=$(mktemp -d) dhall freeze --all --cache --inplace "$FILE"
done

I see there are more checks than running dhall freeze on the project, so I've been trying to run release.nix locally. Is nix-build release.nix the right command for this? I'm hoping I'll have enough space left on my disk and still waiting for that command to finish.

@sjakobi

This comment has been minimized.

Copy link
Collaborator

sjakobi commented Nov 6, 2019

I can't help much with the Nix tooling here.

The error at https://hydra.dhall-lang.org/build/43325/nixlog/3 is

diff --recursive ./tests.actual/type-inference/success/preludeB.dhall ./tests.expected/type-inference/success/preludeB.dhall
171a172,186
>     , renderYAML :
>           ∀ ( json
>             :   ∀(JSON : Type)
>               → ∀ ( json
>                   : { array : List JSON → JSON
>                     , bool : Bool → JSON
>                     , null : JSON
>                     , number : Double → JSON
>                     , object : List { mapKey : Text, mapValue : JSON } → JSON
>                     , string : Text → JSON
>                     }
>                   )
>               → JSON
>             )
>         → Text
builder for '/nix/store/0p6pm3hiycx9lac72r124qkjzi4zy294-test-files-lint.drv' failed with exit code 1

So you need to update the preludeB.dhall golden file with the new inferred type of the Prelude's package.dhall.

@Gabriel439

This comment has been minimized.

Copy link
Contributor

Gabriel439 commented Nov 6, 2019

@matheus23: If you already have Nix installed then we have scripts for this purpose underneath the ./scripts directory. Specifically, the script you want is ./scripts/lint-prelude.sh

Also, you don't have to build everything from scratch if you add our shared Nix cache by following the instructions here: https://github.com/dhall-lang/dhall-haskell#nix

@Gabriel439

This comment has been minimized.

Copy link
Contributor

Gabriel439 commented Nov 6, 2019

@matheus23: Sorry, the correct script is actually ./scripts/generate-test-files.sh, although it looks like you already fixed it.

Either way this looks ready to merge. I added you as a collaborator so that you can merge this yourself

@matheus23 matheus23 merged commit 1f2553f into dhall-lang:master Nov 7, 2019
1 check passed
1 check passed
hydra Hydra build #43327 of dhall-lang:799:dhall-lang
Details
@matheus23 matheus23 deleted the matheus23:philipp/render-yaml branch Nov 7, 2019
@Gabriel439

This comment has been minimized.

Copy link
Contributor

Gabriel439 commented Nov 7, 2019

@matheus23: Thank you for doing this! 🙂

SiriusStarr added a commit to SiriusStarr/dhall-lang that referenced this pull request Dec 7, 2019
* Implement a debugging utility JSON/renderYAML for rendering JSON to YAML, similar to JSON/render.

* Add JSON/core.dhall record to use for importing only constructors in JSON/render(YAML) and JSON/omitNullFields.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.