Update SDK, fix now-not-silently-dropped fields #26
Conversation
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
driver/normalizer/normalizer.go
Outdated
| "Name": Var("name"), | ||
| Fields{ | ||
| {Name: "Name", Op: Var("name")}, | ||
| {Name: "ExpandedFromMacro", Optional: "optMacro", Op: Any()}, |
There was a problem hiding this comment.
I don't see optMacro being used. Please add a comment on why it can be dropped.
There was a problem hiding this comment.
It's not used because it's dropped :) It's there any other way to tag a field as optional without using the Optional field?
There was a problem hiding this comment.
Not really. I can add Drop: true to Fields.
There was a problem hiding this comment.
I'm confused - is something expected to be changed with this annotation regarding to latest SDK release with Drop: true?
There was a problem hiding this comment.
Also after reading https://github.com/bblfsh/sdk/pull/369/files#diff-3bd9e60cee55dd92c09d4e11f0b3905bR754 still confused by intent of
tag a field as optional without using the
Optionalfield
and the difference between Optional + Any() vs Drop (presumably +Any())
There was a problem hiding this comment.
The only difference is that it won't declare a state variable as Optional does:
Optional is implied, but the variable won't be created in this case.
There was a problem hiding this comment.
so Optional = if there is X, you HAVE to use X in ... otherwise still match but do nothing where the Drop is more like a promise that we would never use or need it in a target tree?
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
Signed-off-by: Juanjo Alvarez <juanjo@sourced.tech>
|
@dennwc changed the many Any'es to use Drop+Any, PTAL. |
There was a problem hiding this comment.
🤙
But @dennwc @juanjux - please help me understand, is that our preferred strategy right now in driver updates?
Having hard times orienting myself in the tradoffs involved in choosing this approach over e.g adding Map(Part("_", Obj("field": Any(),)) in Preprocessor.
I wish we could have a meeting on this with @creachadair, or something and agree on way of mapping things 😕
I'm happy to schedule a meeting if you all would like to talk about it. I have no obviously-correct answer here, but would be glad to talk it out. I don't think we need to block this PR for it, since it's a larger design question that we probably shouldn't address until we know what the goal is. :) |
Same as the Python and Ruby PRs.