search

attic-labs / noms

The versioned, forkable, syncable database
Apache License 2.0
7.45k stars 267 forks source link

Implement StructEditor #3733

Closed arv closed 6 years ago

arv commented 6 years ago

This is similar to MapEditor

Unlike a Map all the struct data is available in memory so the building is slightly simpler.

Fixes #3602

aboodman commented 6 years ago

I'm really concerned about the ballooning of API complexity. My experience working on Attic was that the best way to work with structs was by way of marshaling. But now we are adding this whole other way to edit them which is "more efficient" but lots more code. People will have to decide which way they want to do it.

It doesn't feel right to me that this mechanism is completely ignorant of marshaling, which is otherwise the easiest way to work with structs in Go.

arv commented 6 years ago

The motivation is in the bug.

I agree that we might want to get rid of the go-routine here though and keep the direct mutation methods on Map/Set/List/Struct for the one off cases.

arv commented 6 years ago

@rafael-atticlabs ^^^

ghost commented 6 years ago

@aboodman What would it look like for this mechanism to be aware of marshalling?

aboodman commented 6 years ago

I don't have an idea right now, but I feel like this is an instance of "wait for good design". That's why I was advocating for us gaining experience with the editor interface before worrying about structs.

ghost commented 6 years ago

I don't feel like dying on this hill. If @aboodman feels strongly, let's just leave this patch outstanding and demote the bug to P1. It doesn't affect format. Presumably, if someone hits memory problems trying to logically update a "column" of some tabular data, we'll hear about it and can revisit.

arv commented 6 years ago

I'll extract the logic for mutating a struct since it is a lot cleaner than the old code.