(re)enable map key override for plain YAML #550
Conversation
pivotaljohn
force-pushed
the
map-key-override-in-plain-yml
branch
from
777e8a1 to
543d369
Compare
pkg/yamlmeta/map_key_overrides.go
Outdated
| keyToIdxs := r.collectIndexesByKey(mapNode) | ||
| idxsToRemove := r.selectRedundantIdxs(keyToIdxs) | ||
| for _, idx := range idxsToRemove { | ||
| mapNode.Items = append(mapNode.Items[:idx], mapNode.Items[idx+1:]...) | ||
| } |
There was a problem hiding this comment.
how about something like this (havent actually ran):
lastItemIndex := map[interface{}]int{}
for idx, item := range mapNode.Items {
lastItemIndex[item.Key] = idx
}
newItems := []*MapItem{}
seenIndex := map[interface{}]struct{}{}
for _, item := range mapNode.Items {
idx, found := lastItemIndex[item.Key]
if !found {
panic("Internal inconsistency: ...")
}
if _, found := seenIndex[item.Key]; !found {
seenIndex[item.Key] = struct{}{}
newItems = append(newItems, mapNode.Items[idx])
}
}
mapNode.Items = newItems
There was a problem hiding this comment.
nice. 👍🏻 Far far simpler, no sorting, etc. And summarizing it sounds a lot like what the desired behavior is: keep only the last.
There was a problem hiding this comment.
You'll notice I attempted something even more conceptually simpler.
There was a problem hiding this comment.
And summarizing it sounds a lot like what the desired behavior is: keep only the last.
well keep only the last in the "space" of the first one -- because its effectively replacing it (vs delete+insert)
pkg/yamlmeta/walk.go
Outdated
| // Visitor performs an operation on the given Node while traversing the AST. | ||
| // Typically defines the action taken during a Walk(). | ||
| type Visitor interface { | ||
| visit(Node) error |
There was a problem hiding this comment.
is this private intentionally? if so, any reason why not make walk private as well?
There was a problem hiding this comment.
Yes, I did this intentionally. I didn't necessarily want to force the consuming programmer to export this function.
But one nice feature of an exported function name is that it's obviously the starting point in a set of functions attached to struct. So, I might just reverse this.
I see no causal connection between whether yamlmeta.Walk() should be exported and Visitor.visit be... In fact, in order for this walker to be useful in other packages, it would need to be exported.
There was a problem hiding this comment.
In fact, in order for this walker to be useful in other packages, it would need to be exported.
yeah, that's what i was nudging towards -- nobody outside of this pkg would be able to impl Visitor if visit stays private.
There was a problem hiding this comment.
+1 this, I stole the walk.go file for the story I'm working on, and struggled to use it without making it Public.
- introduce tree walker/visitor for AST.
pivotaljohn
force-pushed
the
map-key-override-in-plain-yml
branch
from
e5934eb to
affd732
Compare
| expectedDocSet := docSet.DeepCopyAsNode() | ||
| docSet.OverrideMapKeys() | ||
|
|
||
| printer := yamlmeta.NewPrinterWithOpts(os.Stdout, yamlmeta.PrinterOpts{ExcludeRefs: true}) |
There was a problem hiding this comment.
nit: it's a little weird to have os.Stdout. lets throw in nil instead.
There was a problem hiding this comment.
Copy-pasta from parser_test.go. I'll fix more broadly in a separate commit. 👍🏻
Addresses #310