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

Pain Point: Deeply Nested Amends (No Flat Member Syntax) #423

Open
dwbrite opened this issue Apr 12, 2024 · 12 comments
Open

Pain Point: Deeply Nested Amends (No Flat Member Syntax) #423

dwbrite opened this issue Apr 12, 2024 · 12 comments

Comments

@dwbrite
Copy link

dwbrite commented Apr 12, 2024

I'm testing Pkl as a replacement for k8s tools like Kustomize/Helm. So far it's quite nice, but one pain point is modifying deeply nested properties.

As an example, say I have a HomeAssistant pkl package with a default volume claim of 1Gi, which I'd like to bump up to 2Gi.

import "ha-app.pkl" as homeassistant // extends AppEnvCluster, sets isLeafModule() = true
// assume the above is a third-party package

// default HomeAssistant application, with one modified property
local ha_app = (homeassistant) {
  statefulSets {
    ["core"] {
      spec {
        volumeClaimTemplates {
          [[metadata.name == "homeassistant-config"]] {
            spec {
              resources {
                requests {
                  ["storage"] = "2Gi"
                } // 9 lines of brackets :')
              }
            }
          }
        }
      }
    }
  }
}

output {
  files { // for use by argocd
    ["homeassistant.yaml"] = ha_app.output
  }
}

18 LoC for a single deeply nested amendment seems a little excessive.
One way I've found to improve this is to remove some newlines, but I don't find this ideal.

// I guess this is better?
local ha_app = (homeassistant) { statefulSets { ["core"] { spec { volumeClaimTemplates {
  [[metadata.name == "homeassistant-config"]] { spec { resources { requests {
    ["storage"] = "2Gi"
  }}}}
}}}}}

I could create late-binding variables in a new pkl module which extends ha-app further, but that seems like I'd just be moving the problem down the line.


Personally I'd like to see dot syntax supported for amendments. In my case, that might look something like:

local ha_app = (homeassistant) {
  statefulSets["core"].spec.volumeClaimTemplates {
    [[metadata.name == "homeassistant-config"]] {
      spec.resources.requests["storage"] = "2Gi"
    }
  }
}

Am I missing a more idiomatic solution?

@bioball
Copy link
Contributor

bioball commented Apr 12, 2024

We'd like to add support for flat member syntax (the solution that you suggested) at some point. I definitely agree that this a pain point.

@dwbrite dwbrite changed the title Pain Point: Deeply Nested Amends }}}}}}}}} Pain Point: Deeply Nested Amends / Flat Member Syntax Apr 12, 2024
@dwbrite dwbrite changed the title Pain Point: Deeply Nested Amends / Flat Member Syntax Pain Point: Deeply Nested Amends (No Flat Member Syntax) Apr 12, 2024
@dwbrite
Copy link
Author

dwbrite commented Apr 12, 2024

That's great to hear, and thanks for the quick response! Feel free to change the title to something more suitable for tracking flat member syntax.

Fwiw, Pkl has been fantastic for me, and I often find myself telling people how amazing it is :)
I'm hoping I can bring it into my day job soon 🤞

@translatenix
Copy link
Contributor

translatenix commented Apr 12, 2024

foo.bar.baz.qux {...} looks very similar to expressions foo.bar.baz.qux and (foo.bar.baz.qux) {...}, which complicates parsing and reading. I think a different syntax should at least be considered.

@holzensp
Copy link
Contributor

Actually, the (...) in object amends syntax disambiguates that entirely. A slightly trickier thing about flat member syntax is; what do you do with consecutive amends;

foo.bar.baz = 0
foo.bar.qux.quux = "hello"
foo.bar.qux.corge = "world"

Is that the same as

foo {
  bar {
    baz = 0
    qux {
      quux = "hello"
      corge = "world"
    }
  }
}

or

foo {
  bar {
    baz = 0
  }
} {
  bar {
    qux {
      quux = "hello"
    }
  }
} {
  bar {
    qux {
      corge = "world"
    }
  }
}

That has implications for what super means (and for performance, but that's a shortcoming of the current implementation - albeit non-trivial). There are reasons why you're currently not allowed to amend the same property twice. Flat member syntax makes that ambiguous, because the overlap may be partial.

Or... should that ambiguity should syntactically be disallowed and should this require that properties are aggregated in notation, i.e.

foo.bar {
  baz = 0
  qux {
    quux = "hello"
    corge = "world"
  }
}

@bioball
Copy link
Contributor

bioball commented Apr 12, 2024

In both of those cases, super.foo should have the same meaning, no? It means the parent foo value in the prototype chain. super by itself isn't a valid expression.

An interesting problem here is what outer should mean. It would be surprising if these two were the same:

foo.bar.baz = outer.qux
foo {
  bar {
    baz = outer.qux
  }
}

Another one is this:

bar = 15

foo.bar.baz = bar

It would be surprising if it expands to the below snippet, because in the below snippet, the bar within baz = bar references the middle bar, rather than the module-level bar.

bar = 15

foo {
  bar {
    baz = bar
  }
}

@translatenix
Copy link
Contributor

translatenix commented Apr 12, 2024

Actually, the (...) in object amends syntax disambiguates that entirely.

Distinguishing foo.bar.baz.qux {…} from foo.bar.baz.qux requires arbitrary lookahead. It’s also difficult to parse for a human, especially in the middle of a large program. The similarity with (foo.bar.baz.qux) {…} further complicates human parsing.

@bioball
Copy link
Contributor

bioball commented Apr 12, 2024

Distinguishing foo.bar.baz.qux {…} from foo.bar.baz.qux requires arbitrary lookahead. It’s also difficult to parse for a human, especially in the middle of a large program. The similarity with (foo.bar.baz.qux) {…} further complicates human parsing.

I see your point, but I'm not very convinced that this is a problem. This is unambiguous without needing lookahead:

foo.bar.baz.qux { ... }

This does require lookahead (because bar.baz.qux is a valid expression, and expressions are valid object members):

foo {
  bar.baz.qux { ... }
}

But:

  1. Generally I wouldn't expect to see code that looks like this. I would more expect them to be flattened at the top level, as per the first snippet in my post.
  2. Even in lookahead cases, it seems easy to distinguish.

Before shipping this feature, we'd definitely need to play around with this syntax in real world code.

@translatenix
Copy link
Contributor

translatenix commented Apr 12, 2024

The lookahead here seems easy to me.

Arbitrary lookahead can cause problems with syntax highlighters and parser tools. It can also cause performance issues. It’s best avoided if possible.
elves/elvish#664

This is unambiguous without needing lookahead:

I think that’s only the case because typed objects can’t have elements and entries. Is this limitation here to stay?

I would more expect them to be flattened at the top level, as per the first snippet in my post.

I think it’s best to avoid such assumptions when designing a feature. If the feature can be used inside objects, it will be used inside objects.

@bioball
Copy link
Contributor

bioball commented Apr 12, 2024

I think that’s only the case because typed objects can’t have elements and entries. Is this limitation here to stay?

No, it's because expressions are not permitted at the module level. And, this isn't likely to change.

@translatenix
Copy link
Contributor

translatenix commented Apr 12, 2024

No, it's because expressions are not permitted at the module level.

But if typed objects could have elements, it would make a lot of sense to allow expressions (elements) at the module level. This would also work great for rendering formats that support top-level arrays (JSON, etc.). Same for entries.

@translatenix
Copy link
Contributor

translatenix commented Apr 12, 2024

Or... should that ambiguity should syntactically be disallowed and should this require that properties are aggregated in notation,

This might be a good first (and possibly last) step. Good editor support could ease the strictness pain. Definitely worth prototyping.

It would be surprising if it expands to the below snippet, because in the below snippet, the bar within baz = bar references the middle bar, rather than the module-level bar.

Sounds like a simple expansion won't be good enough.

@bioball
Copy link
Contributor

bioball commented Apr 12, 2024

But if typed objects could have elements, it would make a lot of sense to allow expressions (elements) at the module level. This would also work great for rendering formats that support top-level arrays (JSON, etc.). Same for entries.

Even if typed objects eventually allow entries and elements, I'm kind of bearish on this at the module level.

This starts to feel too much like a scripting language:

module foo

something()

somethingElse()

The way to render top-level arrays is to assign to output.value, e.g.

someListing: Listing<String> = new { "hello" }

output {
  value = someListing
  renderer = new JsonRenderer {}
}

Sounds like a simple expansion won't be good enough.

Yeah; we need to explore what the scoping rules should be. And, it might require that we start resolving variables at parse-time (within AstBuilder.java).

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

No branches or pull requests

4 participants