Draft configuration setup #41
Conversation
.github/workflows/go.yml
Outdated
| @@ -0,0 +1,28 @@ | |||
| name: Go | |||
There was a problem hiding this comment.
Let's stay with TravisCI for now 🙂
There was a problem hiding this comment.
yeah.. sorry, it was rather unintentional 😕
config.go
Outdated
| return c.maxInliningSize | ||
| } | ||
|
|
||
| func (c *Config) SetMaxInliningSize(maxInliningSize InlineSize) { |
There was a problem hiding this comment.
| func (c *Config) SetMaxInliningSize(maxInliningSize InlineSize) { | |
| func MaxItemsToInline(maxItems int) Configurator { |
I think the number of elements to inline makes sense to just be an int rather than a separate type.
Also suggest we change this signature (and the other config options) so that callers can construct this like:
m := memviz.New(
MaxItemsToInline(10),
AnotherOption(false))
m.Map(&foo)(i.e. using this pattern: https://dave.cheney.net/2014/10/17/functional-options-for-friendly-apis)
There was a problem hiding this comment.
Sure, awesome! I myself found the setter methods to be unfit for the init.
| return m.mapPtrIface(iVal, inlineable, depth) | ||
| // Collections | ||
| case reflect.Struct: | ||
| if m.config.maxDepth > 0 && depth > m.config.maxDepth { |
There was a problem hiding this comment.
I think we can probably lift this repeated depth check to just run once at the start of this function. If we're taking depth to mean "number of graphviz nodes" then I think we can use:
if depth >= m.config.maxDepth {
return 0, ""
}i.e. if this value is going to cause another node to be created and we've already reached the limit then stop. This is perhaps a bit tricky because of the behaviour of inlineable though.
I'd have to remind myself what exactly this does but I think it's more of a hint than an instruction so accurately calculating depth is tricky.
There was a problem hiding this comment.
I'm pretty sure we have interpreted �������depth differently. 😆 When you say "number of graphviz nodes", do you mean a) in total? or b) the number of graphviz nodes down the embedded tree of the root object?
i.e. if this value is going to cause another node to be created and we've already reached the limit then stop. This is perhaps a bit tricky because of the behaviour of inlineable though.
I think this actually caught my attention during the first testing, and I worked around it. If it's inlineable it will still be rendered, maxdepth "only" has an influence on next level objects. Please go in and test this, I will too, I am unclear on the details 😉
utils.go
Outdated
| // writeDotStringToPng takes a content of a dot file in a string and makes a graph using Graphviz | ||
| // to ouput an image | ||
| // | ||
| // sourced from https://github.com/Arafatk/DataViz/blob/master/utils/utils.go |
There was a problem hiding this comment.
I think I'd rather just include a snippet in the README showing how to use this util function for those that want it rather than duplicating it here e.g.
b := &bytes.Buffer{}
memviz.Map(foo, b)
utils.WriteDotStringToPng(b.String(), "foo.png")There was a problem hiding this comment.
sure, I only used for "visual testing", to be able to confirm the edits I made in the code. I have moved it to the README
| links = append(links, fmt.Sprintf(" %d:f%d -> %d:name;\n", id, index, fieldID)) | ||
| } else { | ||
| // negative value means max depth was reached so don't link over to next node |
There was a problem hiding this comment.
Hmm perhaps mapValue should return something more clear like a bool saying that this was inlined/skipped.
I also think we should render something here to show that the field existed but was skipped.
There was a problem hiding this comment.
Hmm perhaps
mapValueshould return something more clear like a bool saying that this was inlined/skipped.
I agree, this was a quick edit to test out the concept.
I also think we should render something here to show that the field existed but was skipped.
You mean "render" as in within the dot notation? Would that still count as "skipping"?
Hi @bradleyjkemp, this would be my initial take on the config... check it out and let me know