search

ipld / go-ipld-prime

Golang interfaces for the IPLD Data Model, with core Codecs included, IPLD Schemas support, and some handy functional transforms tools.
MIT License
133 stars 50 forks source link

Nodes need a pretty-print convention. #88

Closed warpfork closed 3 years ago

warpfork commented 4 years ago

I'm trying to write some examples for docs, and the output is... erm.

Node implementation internals tend to be full of details that are not useful to present to users. For example, a basicnode handed to fmt.Printf("%v", n) will spout out some nonsense like &{map[k1:0xc000097ef0 k2:0xc000097f00 k3:0xc0000e0e60] [{k1 0xc000097ef0} ...

We need some stringers.

I think the way to go about this is probably two-step:

When we're writing examples for documentation, I'm not sure if we'll then prefer to call that codec with a node argument, or just plain Printf the node in question, but we can figure out which of those is preferable later. I kinda figure I want "%v" to not be a mess of pointers, anyway; that's just not helpful.

I said "codec" above, but I don't think this has to be a multicodec or anything. It's probably fine if it's an implementation detail of this library and not standardized across IPLD. And I really don't care if it's parseable again at all; emit-only is fine for this usecase. The only thing that makes it like a "codec" is that it's going to want to recurse over nodes in roughly the same way, and it emits a stream of bytes to an io.Writer.

The closest workaround right now: is to emit dag-json, or, if you want control over the whitespace (which, yes please, for the use case of human-readable examples!), do something terribly custom like this: https://github.com/ipld/go-ipld-prime/blob/350032422383277e6545b9b1a49112123b5c43fb/schema/gen/go/testcase_test.go#L209-L212

These work, as a workaround, but there's two drawbacks to this:

warpfork commented 4 years ago

I might start working on this immediately. I'm nerdsniped, and I've stubbed my toe on this repeatedly; I don't want to keep stubbing my toe on it every time I try to write good new examples for the godocs.

mvdan commented 4 years ago

Some thoughts:

I think fmt.Stringer would be the simplest, and I would argue that we should strongly recommend that nodes implement a String method anyway. It's just too useful for quick debugging.

Like you say, it gets tricky with complex types like structs or maps. Before you implement anything, have you tried something like https://godoc.org/github.com/kr/pretty#Println? If it's close to being good enough, we could likely implement something very similar that's IPLD-specific and does a better job for our needs. The big advantage here over String being that complex structures would be able to be shown in a prettier way, with newlines and indentation that can be configured.

Then there's fmt.GoStringer like you say, but IMO we should just automate that via a reflection API like kr/pretty. It makes little sense to me to have every node implement this themselves.

warpfork commented 4 years ago

I'll give a quick eyeball to that pretty library, but I think in this case we'll probably do something ourselves, yeah. Customizability is probably not a virtue for this; I'd rather not take on a transitive dependency for this; and also, I'm kind of hoping to use this as an opportunity to do a little research spike in the direction of https://github.com/ipld/go-ipld-prime/issues/86 .

I think I might do a little feature-detection for nodes matching the schema.TypedNode interface, and annotate their printout with type info in a standard way, too. (This'll make the result even more clearly not a multicodec, because that kind of info would be verboten to emit or expect to read in a multicodec, because it would violate layering, but it's already established this wouldn't be a multicodec so that should be okay.)

mvdan commented 4 years ago

Sounds good to me. My only strong opinion here is that the bulk of the code should be generic, and that a Node implementation shouldn't need extra methods to support basic pretty-printing. Reflection here shouldn't be a problem at all, because pretty-printing is for humans.

rvagg commented 4 years ago

As mentioned on Slack, I'd like to be able to use something like this for portable test fixtures, so it'd be good if the output was generic enough that it was simple to implement in other languages. I've been thinking that a pretty-print version of dag-json might be the most sensible way forward here because we have dag-json in most places that matter and those implementations should be straightforward to hack to prett-print. It's these darn CIDs and Bytes that get in the way and we already have a convention in dag-json so I guess we ought to re-use that.

rvagg commented 4 years ago

dag-json-pretty could be a legit codec with strong rules about whitespace .. btw.

warpfork commented 3 years ago

https://github.com/ipld/go-ipld-prime/issues/191 is tangentally related to this. tl;dr: There are some under-answered questions about what a good UX is around absent values when typed nodes are in play.

It also means that trying to implement a pretty-print using some build-a-codec helper functions (as https://github.com/ipld/go-ipld-prime/issues/86 might dream of) is fraught with a few elements of peril, because it's looking like codecs generally shouldn't handle Absent (and should error if they see it), whereas a pretty-print should handle Absent in order to be maximally useful.

warpfork commented 3 years ago

Delightfully, I feel that I can now call this closed, due to the introduction of #238.