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

Feature Request: Non-Recursive Deep Record Merges #340

Closed
ari-becker opened this issue Jan 8, 2019 · 4 comments
Closed

Feature Request: Non-Recursive Deep Record Merges #340

ari-becker opened this issue Jan 8, 2019 · 4 comments

Comments

@ari-becker
Copy link
Collaborator

@ari-becker ari-becker commented Jan 8, 2019

broken.yaml:

let preexistingDefaultRecord = { meta   = { name = "name"
                                          , version = "version"
                                          }
                               , config = { propertyA = "default"
                                          , propertyB = "default"
                                          }
                               }

let newRecord = preexistingDefaultRecord // { config = { propertyB = "new" } }

in newRecord

Output of dhall-to-yaml < broken.dhall:
Actual (complete overwriting of config):

config:
  propertyB: new
meta:
  name: name
  version: version

Desired (only config.propertyB is overwritten):

config:
  propertyA: default
  propertyB: new
meta:
  name: name
  version: version

Desired expression for new functionality: preexistingDefaultRecord /// { config = { propertyB = "new" }}

Use case : it's common to evaluate a Dhall expression with a type of a complex, deeply nested record, where many of the deeply nested values are of type Optional. To avoid needing to specify large amounts of Optional boilerplate, for each instantiation of such a complex type, it is common to create a function which creates a "default" instantiation which sets all of the Optional fields to None (commonly in dhall-kubernetes). However, to set one or a few of the deeply nested Optional fields, it is required to either "unpack" the nested record, set the relevant field, and then non-recursively merge the entire nested record, or not to use a "default" function and instead write (fragile, in case of type changes) boilerplate setting many of the fields to None.

Current workaround:

let preexistingDefaultRecord = { meta   = { name = "name"
                                          , version = "version"
                                          }
                               , config = { propertyA = "default"
                                          , propertyB = "default"
                                          }
                               }

let newConfig = preexistingDefaultRecord.config // { propertyB = "new" }
let newRecord = preexistingDefaultRecord // { config = newConfig }

in newRecord
@Gabriel439
Copy link
Contributor

@Gabriel439 Gabriel439 commented Jan 8, 2019

@ari-becker: The main reason that Dhall doesn't have this feature is that it would complicate the standard semantics because you would now have to interleave type-checking and normalization.

The secondary reason is that even with support for this it's not sufficiently general to handle cases where the inner record is nested within a List or Optional. In those cases you still have to provide the user with a default value for the inner record.

See a similar discussion here: #114 (comment)

However, what I can do is document this common idiom in the FAQ

@ari-becker
Copy link
Collaborator Author

@ari-becker ari-becker commented Jan 9, 2019

@Gabriel439 I do think that it would be a good idea to document the idiom that you pointed out in #114 in the FAQ, which, while unwieldy, still attempts to address the issue of how to selectively insert values in complex nested records.

In general, I think that effort should be paid to trying to improve the experience of working with defaults for types, which is where this feature request is really coming from.

@Gabriel439
Copy link
Contributor

@Gabriel439 Gabriel439 commented Jan 11, 2019

@ari-becker: Yeah, one way I'm thinking about approaching this problem is adding built-in support for nested-updates (i.e. similar to Haskell's lenses/traversals), rather than changing the behavior of the // operator. For example, you could imagine writing:

set config.propertyB "new" preexistingDefaultRecord

... or if the record containing propertyB were nested inside of a List you could update all of the elements by writing:

set config.*.propertyB "new" preexistingDefaultRecord

However, that's a large enough extension to the language that I'm holding off on it until there are more language bindings

Gabriel439 added a commit that referenced this issue Feb 25, 2020
Fixes #114

Fixes #340

Related to #754

This adds a new keyword which is syntactic sugar for using `//` to
perform a deep override of the record.  This comes in handy when
working with very large records (such as those found in
`dhall-kubernetes`).  This similar in spirit to Kustomize, albeit not
as powerful and customizable as Kustomize.

The goal is that the user can write the following code to update an
existing default configuration:

```dhall
someDeployment with {
, metadata.labels =
      someDeployment.metadata.labels
    # toMap { `scheduler.alpha.kubernetes.io/critical-pod` = "" }
}
```

You can also see this idiom outside of the Dhall/Kuberentes ecosystem.
For example, many Nix users manually apply deeply nested updates by
hand, like this example from the Nixpkgs manual:

```nix
  {
    haskell = super.haskell // {
      packages = super.haskell.packages // {
        ghc784 = super.haskell.packages.ghc784.override {
          overrides = self: super: {
            ghc-events = self.callPackage ./ghc-events-0.4.3.0.nix {};
          };
        };
      };
    };
  };
```

... where the Dhall equivalent would be:

```dhall
super with
  { haskell.packages.ghc784 = super.haskell.packages.ghc784.override {
      …
    }
  }
```

There is one additional change that I plan to make that builds on top of
this which is to would like to have the record completion syntax desugar
to use `with` instead of `//`, like this:

```dhall
T::r = (T.default with r) : T.Type
```

... which would allow for deep overrides for the record completion
operator, too.  However, I chose to defer that to a separate change to
keep this change as minimal as possible.
@Gabriel439
Copy link
Contributor

@Gabriel439 Gabriel439 commented Feb 25, 2020

The fix for this is up here: #923

Gabriel439 added a commit that referenced this issue Mar 3, 2020
Fixes #114

Fixes #340

Fixes #754

This adds a new keyword which is syntactic sugar for using `//` to
perform a deep override of the record.  This comes in handy when
working with very large records (such as those found in
`dhall-kubernetes`).  This similar in spirit to Kustomize, albeit not
as powerful and customizable as Kustomize.

The goal is that the user can write the following code to update an
existing default configuration:

```dhall
someDeployment
  with metadata.labels =
      someDeployment.metadata.labels
    # toMap { `scheduler.alpha.kubernetes.io/critical-pod` = "" }
```

You can also see this idiom outside of the Dhall/Kuberentes ecosystem.
For example, many Nix users manually apply deeply nested updates by
hand, like this example from the Nixpkgs manual:

```nix
  {
    haskell = super.haskell // {
      packages = super.haskell.packages // {
        ghc784 = super.haskell.packages.ghc784.override {
          overrides = self: super: {
            ghc-events = self.callPackage ./ghc-events-0.4.3.0.nix {};
          };
        };
      };
    };
  };
```

... where the Dhall equivalent would be:

```dhall
super
  with haskell.packages.ghc784 = super.haskell.packages.ghc784.override { … }
```

There is one additional change that I plan to make that builds on top of
this which is to would like to have the record completion syntax desugar
to use `with` instead of `//`, like this:

```dhall
T::r = (T.default with r) : T.Type
```

... which would allow for deep overrides for the record completion
operator, too.  However, I chose to defer that to a separate change to
keep this change as minimal as possible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants