[POC] Add 'quoted' option #941

erthalion · 2019-05-07T09:53:06Z

Allow to generare quoted scalars if needed via providing a custom encode
options to Data.Yaml.encodeWith. So far two corner cases from yaml
itself (an empty scalar, and special strings) are omitted in the
implementation.

Allow to generare quoted scalars if needed via providing a custom encode options to Data.Yaml.encodeWith. So far two corner cases from yaml itself (an empty scalar, and special strings) are omitted in the implementation.

`yaml-0.11.0` split out the `Text.Libyaml` module into a separate `libyaml` Haskell package. However, that means that if you depend on the `libyaml` package then you can no longer use older versions of the `yaml` package because then they will both conflict due to trying to export `Text.LibYaml`. That presents one of two options: * Upgrade `dhall-json` builds for older resolvers to use `yaml-0.11.0` * Downgrade `dhall-json` builds for newer resolvers to use `yaml-0.10.4.0` I went with the latter change since this only affects the `stack.yaml` build and is fairly non-disruptive, whereas upgrading `stack-lts-6.yaml` to use a newer `yaml` was likely to be more disruptive. By pinning to an older version of the `yaml` package we no longer need the `libyaml` dependency in `dhall-json.cabal`, which this change also removes.

CI builds with `-Werror`, so any warnings fail the builds. This change fixes two warnings: * An unused variable warning for the `customStyle` input argument: `s` * A missing type signature for `customStyle` as a top-level function

Gabriella439 · 2019-05-07T17:05:04Z

@erthalion: If you merge erthalion#1 into your branch then I think it will fix the build failures in CI

Fix build failures in CI

Too simple custom style can break "as Text" import, since it's also going to be quoted. So it needs to be in the condition as Literal.

Gabriella439 · 2019-05-08T02:41:31Z

@erthalion: Hmmm. It looks like you will need to upgrade stack-lts-6.yaml and stack-lts-12.yaml to use yaml-0.10.4.0 in order to pass the AppVeyor build because older yaml versions don't support customizing the rendering

Add the version that supports string rendering styles to stack-lts-12/6

erthalion · 2019-05-08T13:12:34Z

@Gabriel439 Right, it fixes tests for stack-lts-12.yaml. But looks like for stack-lts-6.yaml this leads to:

Error: While constructing the build plan, the following exceptions were encountered:
In the dependencies for yaml-0.10.4.0:
    base-4.8.2.0 from stack configuration does not match >=4.9.1 && <5  (latest matching version
                 is 4.12.0.0)
needed due to dhall-json-1.2.8 -> yaml-0.10.4.0

I'm not sure, is there a better way to fix it, except allow-newer: true?

Gabriella439 · 2019-05-08T15:19:30Z

@erthalion: One solution would be to disable the dhall-json build for stack-lts-6.yaml. The only reason we test against LTS 6 is for Eta's Etlas package manager (which supports Dhall as a configuration language and is built using LTS 6), but that only needs dhall to build and not dhall-json. (cc: @jneira)

Another solution would be to use CPP to conditionally compile out the --quoted option feature (i.e. #if MIN_VERSION_yaml(0,10,4)) so that it can build against older versions of yaml

You can also try allow-newer: true, but my guess is that yaml's lower bound on base exists for a reason and all that will happen if you do that is that the yaml package build will fail.

My personal preference is the first solution (discontinue dhall-json support for LTS 6)

jneira · 2019-05-08T19:43:02Z

Well, really the component needed for etlas is only dhall-haskell, so we could remove dhall-json from lts-6.
Otoh eta is somewhat between ghc-7.10 and ghc-8.x, so packages buildables with 7.10 usually can be built with eta (but not always the other way around).
In this case the change would not impact eta cause it uses base-4.11.1.0 and cant use yaml anyway cause it is based in c ffi, so the package has to be patched.
(Btw i am working in a dhall-json version that could be built with ghc, eta and hopefully ghcjs)

erthalion · 2019-05-10T15:15:36Z

My personal preference is the first solution (discontinue dhall-json support for LTS 6)

so we could remove dhall-json from lts-6

Stupid question, how to do this? I thought it would be enough just to remove dhall-json from packages in stack-lts-6.yaml, but looks like it's not.

Gabriella439 · 2019-05-10T15:28:19Z

@erthalion: The appveyor.yml script needs to be changed to not attempt to package dhall-json into a TAR archive for stack-lts6.yaml. I'm still new to appveyor myself, but I believe this entails separating out the package step into a block that doesn't run for stack-lts6.yaml, similar to what is done for the tests here:

dhall-haskell/appveyor.yml

Lines 52 to 64 in 2530207

    
           for: 
        
           - 
        
             matrix: 
        
               except: 
        
                 # stack is not able to build test dependencies with lts-6 
        
                 - STACK_YAML: stack-lts-6.yaml 
        
             test_script: 
        
               - stack test dhall:tasty 
        
               - stack test dhall-json 
        
               - stack test dhall-text 
        
               - stack test dhall-bash 
        
               - stack test dhall-lsp-server

erthalion · 2019-05-11T17:30:39Z

Indeed, @Gabriel439 it worked, thanks for the explanation. I'm going to write a few real tests now.

erthalion · 2019-05-12T09:34:09Z

To make it testable I've introduced jsonToYaml function in the dhall-json library itself (which brought yaml dependencies there), is it acceptable?

For the testing purposes yaml generation logic is moved to the library itself. Tests are provided for quoted and normal style, including plain text values.

Gabriella439 · 2019-05-12T14:38:40Z

@erthalion: Yeah, that's totally fine. I usually treat the executable and library dependencies as essentially synonymous. The only dependencies I treat differently are test and benchmark dependencies (since they are not always used).

Also, for this package we can be quite liberal with dependencies since it's most commonly use as an executable rather than as a package.

Gabriella439 · 2019-05-12T14:43:47Z

Excellent job! Thank you for doing this 🙂

erthalion · 2019-05-12T15:26:15Z

Thank you!

Gabriella439 · 2019-05-12T15:28:09Z

You're welcome!

[POC] Add 'quoted' option

b2ae48f

Allow to generare quoted scalars if needed via providing a custom encode options to Data.Yaml.encodeWith. So far two corner cases from yaml itself (an empty scalar, and special strings) are omitted in the implementation.

erthalion mentioned this pull request May 7, 2019

Double quotes with a text value #933

Closed

Gabriella439 added 2 commits May 7, 2019 10:02

Fix warnings

eb7a3f8

CI builds with `-Werror`, so any warnings fail the builds. This change fixes two warnings: * An unused variable warning for the `customStyle` input argument: `s` * A missing type signature for `customStyle` as a top-level function

erthalion added 3 commits May 7, 2019 20:00

Merge pull request #1 from dhall-lang/gabriel/fix_yaml_ci

45d74f1

Fix build failures in CI

Merge branch 'master' into feature/quoted-scalars

21fa181

Generate Literal for "as Text"

55f83a0

Too simple custom style can break "as Text" import, since it's also going to be quoted. So it needs to be in the condition as Literal.

Add yaml-0.10.4.0

9954f02

Add the version that supports string rendering styles to stack-lts-12/6

Remove dhall-json from stack-lts-6

f4865e9

erthalion force-pushed the feature/quoted-scalars branch 3 times, most recently from 530498d to afefbb9 Compare May 11, 2019 16:53

Skip dhall-json for LTS 6 build

afefbb9

Gabriella439 mentioned this pull request May 12, 2019

Change YAML/JSON encoding for NaN/Infinity/-Infinity #946

Merged

Merge branch 'master' into feature/quoted-scalars

909aa0e

erthalion force-pushed the feature/quoted-scalars branch from 3b4e40d to 641e869 Compare May 12, 2019 09:36

Add a few tests for quoted style

641e869

For the testing purposes yaml generation logic is moved to the library itself. Tests are provided for quoted and normal style, including plain text values.

Gabriella439 merged commit 0790667 into dhall-lang:master May 12, 2019

This was referenced May 29, 2019

dhall-json: yaml-pre-0_11 does not build #969

Closed

Migrate to the HsYaml package #744

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[POC] Add 'quoted' option #941

[POC] Add 'quoted' option #941

erthalion commented May 7, 2019

Gabriella439 commented May 7, 2019

Gabriella439 commented May 8, 2019

erthalion commented May 8, 2019

Gabriella439 commented May 8, 2019

jneira commented May 8, 2019

erthalion commented May 10, 2019 •

edited

Loading

Gabriella439 commented May 10, 2019

erthalion commented May 11, 2019

erthalion commented May 12, 2019

Gabriella439 commented May 12, 2019

Gabriella439 commented May 12, 2019

erthalion commented May 12, 2019

Gabriella439 commented May 12, 2019

[POC] Add 'quoted' option #941

[POC] Add 'quoted' option #941

Conversation

erthalion commented May 7, 2019

Gabriella439 commented May 7, 2019

Gabriella439 commented May 8, 2019

erthalion commented May 8, 2019

Gabriella439 commented May 8, 2019

jneira commented May 8, 2019

erthalion commented May 10, 2019 • edited Loading

Gabriella439 commented May 10, 2019

erthalion commented May 11, 2019

erthalion commented May 12, 2019

Gabriella439 commented May 12, 2019

Gabriella439 commented May 12, 2019

erthalion commented May 12, 2019

Gabriella439 commented May 12, 2019

erthalion commented May 10, 2019 •

edited

Loading