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

folding syntax #382

Open
sh0rez opened this issue Feb 21, 2020 · 8 comments
Open

folding syntax #382

sh0rez opened this issue Feb 21, 2020 · 8 comments

Comments

@sh0rez
Copy link
Contributor

sh0rez commented Feb 21, 2020

I'd like to propose a folding syntax for single valued objects, loosely inspired by CUE: https://cuelang.org/docs/tutorials/tour/intro/fold/

It's frequently the case that when I want to override a specific property of something nested very deeply, I need to write a lot of encapsulated curly braces, which wastes a lot of lines and screen space.
The other option of folding these and having something like }}}}}}} doesn't look much better either, and is error prone.

Instead, we could allow this:

{
  abc+: def+: xyz+: {
    foo: "bar"
  }
}

which would desugar into this:

{
  abc+: {
    def+: {
      xyz+: {
        foo: "bar"
      }
    }
  }
}

I think this would make several scenarios much more concise. WDYT @sbarzowski @sparkprime?

@sbarzowski
Copy link
Collaborator

sbarzowski commented Feb 21, 2020

First of all, frequently overriding deeply nested fields is something I would consider a bad smell. It's often better to build the right objects in one go (and when you have multiple variants have a function which builds them based on the parameters you pass to it). That's just my opinion, though.

You may also want to try this little library: https://github.com/sbarzowski/jsonnet-modifiers, which addresses the problem of deep modifications.

Going back to your proposal - I'm ok with it. This particular syntax probably wouldn't work, because we expect an expression after +:, so we need to come up with something else (or with the right rule to distinguish between expression and a continued "folding" syntax).

Perhaps it would be easier and more concise to have the syntax as follows:

{
  abc.def.xyz +: {
    foo: "bar"
  }
}

or with escaped strings:

{
  "  "."\t"."foo bar" +: {
    foo: "bar"
  }
}

or with general expressions:

{
  [e1].[e2].[e3]+: {
    foo: "bar"
  }
}

That should work syntactically, but I'm a bit skeptical about allowing the calculated field names with this syntax, at least in the first version.

I feel strongly that we should not allow "entering the field multiple times". So this would not be allowed:

{
    a.foo +: aaa
    a.bar +: bbb
}

(The example above was edited, thanks @sh0rez for pointing out the typo.)

You would need to do it the normal way (so that all modifications to the same field are grouped in one operation)

{
    a +: { foo: aaa, bar: bbb }
}

A disadvantage compared to your syntax is that you allow making a different choice of :, :: and ::: at every level.

@sh0rez
Copy link
Contributor Author

sh0rez commented Feb 21, 2020

Perhaps it would be easier and more concise to have the syntax as follows:

{
  abc.def.xyz +: {
    foo: "bar"
  }
}

Considered that as well, but the downside I see with this approach is that we lose control over the colon operator at each level. With that, will it be +: all the time? how do I get a :: between abc and def?

@sh0rez
Copy link
Contributor Author

sh0rez commented Feb 21, 2020

I feel strongly that we should not allow "entering the field multiple times". So this would not be allowed:

{
    a.foo +: aaa
    b.bar +: bbb
}

You would need to do it the normal way (so that all modifications to the same field are grouped in one operation)

{
    a +: { foo: aaa, bar: bbb }
}

These examples are not equivalent ..


I agree that this should be disallowed:

{
  foo+: bar+: a: "hi",
  foo+: bar+: b: "ho",
}

because obviously foo is defined twice. Instead:

{
  foo+: bar+: {
    a: "hi",
    b: "ho",
  }
}

@sh0rez
Copy link
Contributor Author

sh0rez commented Feb 21, 2020

First of all, frequently overriding deeply nested fields is something I would consider a bad smell. It's often better it to build the right objects in one go (and when you have multiple variants have a function which builds them based on the parameters you pass to it). That's just my opinion, though.

At least for our use of Tanka, this does not hold true in all cases. We have chosen Jsonnet in the first place, because +: allows us to fix things late, even if the library is not configurable enough.

Our primary use case are environments, where we have an abstract form of it somewhere on the import paths (the base), and for each instance of that we customize it using +:.

@sbarzowski
Copy link
Collaborator

sbarzowski commented Feb 21, 2020

With that, will it be +: all the time?

Yes, that's how I was thinking about it. Most of the time that that would be the right thing I guess, because this wouldn't change the visibility the fields already have.

how do I get a :: between abc and def?

Then you would need to fall back to the old syntax. It may be good anyway to make things explicit when visibilities are mixed.

@sh0rez
Copy link
Contributor Author

sh0rez commented Feb 21, 2020

Okay, that's a fair tradeoff.

Visually I prefer my proposed one, but if the other one makes implementation much simpler we can go with it instead!

@sh0rez
Copy link
Contributor Author

sh0rez commented Mar 21, 2020

@sbarzowski I'd like to give this a try, but I am not very familiar with the Jsonnet codebase ... could you give me some tips where to look at and how a good implementation would look in your eyes?

@sbarzowski
Copy link
Collaborator

This is a change to the core language so step one is to make sure that @sparkprime is ok with this :-).

The following changes are necessary:

  • AST - the representation of the new syntax. This should be "lossless", that is the representation should reflect 1-1 the source code (so that it can be safely unparsed in the formatter). Here is the representation of lossless object fields: https://github.com/google/go-jsonnet/blob/v0.15.0/ast/ast.go#L576. It will probably need to change quite a lot, so it may be a time to refactor it. I guess the current field kinds would translate to "field components" and we can the "field path" as a slice of components (e.g. a.b.c: "foo" may have path []FieldComponent{FieldID{a}, FieldID{b}, FieldID{c}}. (This is just a quick idea, it is not the only way and it may not be the best way).
  • Parser - actually parse this new syntax and save it as AST. We should check for duplicates ({a.b = "foo", a.c="bar"}) at this stage. Parsing of fields happens here: https://github.com/google/go-jsonnet/blob/v0.15.0/internal/parser/parser.go#L393 (local and assert fields are parsed elsewhere).
  • Desugarer - At this stage we translate lossless AST to desugared, simplified AST for evaluation. Here we expand the nested fields. At this point we should not report any errors. Here you can find field desugaring: https://github.com/google/go-jsonnet/blob/v0.15.0/internal/program/desugarer.go#L127
  • Tests - just some example files using this syntax. It should cover corner cases (e.g. including like `a.["foo"]."bar").
  • Spec change - we maintain a formal description of the language (https://jsonnet.org/ref/spec.html). This is important for tooling and language development. Grammar and desugaring rules will need to be updated. I can do this if you are not comfortable with the mathy specs.
  • Tutorial change (or some other newbie-friendly resource) - so that the users can discover this feature.

Unfortunately first three need to be done for both cpp-jsonnet and go-jsonnet.

I'll be happy to answer any questions or help if you run into any problems with this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants