Printer feature #238
Printer feature #238
Conversation
In simple cases, anyway.
| n := bindnode.Wrap(&FooBar{"x", "y"}, ts.TypeByName("FooBar")) | ||
| qt.Check(t, Sprint(n), qt.CmpEquals(), wish.Dedent(` | ||
| struct<FooBar>{ | ||
| foo: string<String>{"x"} |
There was a problem hiding this comment.
string<String> is getting weird, I don't suppose it's reasonable to special-case the typed data model kinds like String to omit the typing here?
There was a problem hiding this comment.
Yeah I'm on the fence on this. At first I thought I'd strip it, as you suggest. Then I looked at the printout the first time I got one to come out, and was like... hum: typed vs untyped string seems like the kind of distinction that could actually trip someone up someday, so maybe that's useful.
I'm still on the fence, a couple days later. I can't think of how typed-vs-untyped string would trip someone up, exactly, either. It's just a nagging fear.
Maybe a config option for "SmartElidePreludeTypeInfo bool"?
There was a problem hiding this comment.
I think I'd propose we leave it in verbose mode for now, someday add that config flag, and then if enough people are perturbed by the verbosity we flip the default of that flag to true. We should probably get some impressions from people further out from us before deciding to elide things that might be obvious to us but nonobvious to new users, is my thinking.
There was a problem hiding this comment.
Added a config placeholder for this and attached documentation for it so someone looking for such a feature can at least find that we've considered it. Haven't implemented it immediately, however.
printer/doc.go
Outdated
| } | ||
| "unit types": unit<TheTypeName> | ||
| "notice unit types": "have no braces at all, because they have literally no further details. they're all type info." | ||
| "variants": variant<TheUnionName>{string<TheInhabitant>{ |
There was a problem hiding this comment.
so variant is the name for "specific variant of a union"? introduction of new terminology is a concern I reckon, it doesn't read as smoothly if there's disconnect with user-consumed schema lingo
There was a problem hiding this comment.
oh oof, I should s/variant/union/ this. It's a stretchmark from how I think during development: I was trying out "variant" as an alternate name for "union" and trying to get a feel for if I liked it when seeing and using it for a while.
The result is "yes", I do kinda come to prefer that term. I think maybe we should consider doing a mass rename of "union" to "variant".
But that's not a discussion I want to force through in this PR; it deserves its own space.
|
I think I prefer aside from comments left inline, I think I like this. I did something a little bit similar in a JS experiment I was working on but this reads much better and you've integrated type information which is great, so I might have to copy this. |
|
🙌 I'm thrilled you like it. And you're right, I think |
The defaults and what we are explicit about is rather different (mostly, more) than what I originally expected. Bring the example back in line with where the implementation actually ended up. (We *are* going to make strings annotated by default, per review: #238 (comment) )
…e config option. 'ElidePreludeTypeInfo' originates from code review discussion: #238 (comment)
|
Though this contains some TODOs, I'm going to go ahead and merge it. Since this seems directionally acceptable, it should be no problem to add the remaining features in subsequent PRs. (I've gotten myself in the awkward position of wanting both this, and some other features recently landed, at the same time, in a downstream, and yet don't have the time block today to finish the remaining feature wishlist in this diff. So: onward!) |
I've started developing a
printerfeature, which provides a human-readable debugging output describing a tree of nodes.This is an answer to #88 .
datamodel.Nodeinterface, and feature-detectsschema.TypedNodeas appropriate.The syntax is somewhat impulsively chosen. Roughly, it's
kindname{"value"}(so:string{"foo"}andint{12}andbool{true}etc), or for typed information,typekindname<TheTypeName>{"value"}. You can check out the tests for a few more examples of how it looks. We could bikeshed this syntax, I suppose, but so far, I'm finding it tolerably readable.I've started using this in debugging myself already as I work using go-ipld-prime as a downstream user in other projects, and it's saving me tons of time, and also starting to show up in my docs there to explain features to people. I'm pretty happy about it. (Also: it means I'm already relatively attached to some of these commit hashes and will try to roll forward and resist a need to rebase if possible.)
In the future, I'd like to add some functions to the root
ipldpackage which call these printer features, for rapid easy use and for discoverability, but that's not included in this PR yet.Review welcome. This seems like the sort of thing I'd rather merge relatively quickly, and continue iterating on master, though.