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

Nodes need a pretty-print convention. #88

Closed
warpfork opened this issue Oct 1, 2020 · 8 comments
Closed

Nodes need a pretty-print convention. #88

warpfork opened this issue Oct 1, 2020 · 8 comments

Comments

@warpfork
Copy link
Collaborator

warpfork commented Oct 1, 2020

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:

  • implement a pretty-print "codec" of some kind,
  • and then make it highly recommended that Node implementations add a method to match Stringer (or GoStringer?) which calls that pretty-print codec.

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:

// We'll marshal with "pretty" linebreaks and indents (and re-format the fixture to the same) for better diffing.
prettyprint := json.EncodeOptions{Line: []byte{'\n'}, Indent: []byte{'\t'}}
var buf bytes.Buffer
err := dagjson.Marshal(n, json.NewEncoder(&buf, prettyprint))

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

  • the way the latter option is reaching out to the refmt library is a little perturbing
  • it's a huge distraction to be getting concrete about dag-json if I'm trying to write an example that's purely about the Data Model.
@warpfork
Copy link
Collaborator Author

warpfork commented Oct 1, 2020

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
Copy link
Contributor

mvdan commented Oct 1, 2020

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
Copy link
Collaborator Author

warpfork commented Oct 1, 2020

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 #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
Copy link
Contributor

mvdan commented Oct 1, 2020

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
Copy link
Member

rvagg commented Oct 1, 2020

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
Copy link
Member

rvagg commented Oct 1, 2020

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

@warpfork
Copy link
Collaborator Author

warpfork commented Jun 9, 2021

#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 #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
Copy link
Collaborator Author

warpfork commented Sep 8, 2021

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

@warpfork warpfork closed this as completed Sep 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants