Conversation
|
fixes #312 |
bb91bd3 to
0e96031
Compare
|
Now also with a little bit of resource types: It's ugly atm, eg: no proper indicator for readonly types. |
purpleidea
left a comment
There was a problem hiding this comment.
Sweet! Two broad design questions:
-
Shouldn't resource output contain the types in our type system equivalent instead of what they actually are?
-
Can we add a
--xmlflag to produce machine readable output too? We can postpone for a later patch though.
lib/info.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| w := tabwriter.NewWriter(os.Stdout, 0, 8, 2, ' ', 0) |
There was a problem hiding this comment.
Can we have these vars be const (at the top), if only to improve readability please?
lib/info.go
Outdated
| "github.com/urfave/cli" | ||
| ) | ||
|
|
||
| var myPrintln = fmt.Printf |
There was a problem hiding this comment.
leftover from the wrong approach to capture test output
lib/info.go
Outdated
| switch c.Command.Name { | ||
| case "resources": | ||
| for _, name := range resources.RegisteredResourcesNames() { | ||
| if name == "test" { |
| s := reflect.ValueOf(res).Elem() | ||
| typeOfT := s.Type() | ||
|
|
||
| var fields []string |
There was a problem hiding this comment.
use, improve, or add to one of the functions in resources/util.go please,
if anything wouldn't we want the info sub commands to be separate functions?
lib/info.go
Outdated
| if err != nil { | ||
| continue | ||
| } | ||
| switch fn.(type) { |
There was a problem hiding this comment.
_, ok := fn.(interfaces.PolyFunc)
| assert.Contains(t, out.String(), "load() struct{x1 float; x5 float; x15 float}") | ||
| } | ||
|
|
||
| // func TestPolyFunctionsWithTypes(t *testing.T) { |
There was a problem hiding this comment.
scratchpad until i have polyfuncs figured out or for a 2.0 patch of this feature
| // return nil | ||
| //} | ||
|
|
||
| // Lookup returns a pointer to the resource's struct. |
There was a problem hiding this comment.
Not sure why we need a helper function for this.
There was a problem hiding this comment.
Just mirroring funcs Lookup here
There was a problem hiding this comment.
also registeredResources is private :)
There was a problem hiding this comment.
I didn't realize this was in resources.go :P
lib/info.go
Outdated
| return nil | ||
| } | ||
|
|
||
| // infocmd takes the cli context and returns the requested output |
There was a problem hiding this comment.
golang style says InfoCmd or infoCmd please, but not sure why this is a separate function altogether
There was a problem hiding this comment.
to allow unit testing the output without jumping through hoops to capture it (eg: rerouting stdout, etc)
|
1: the resource rending is more a hack than a preferred implementation, if you know of a way to get the right output i'm all ears 2: if there is an easy library which can write this to XML in a open format suitable for this kind of data I will add it in, otherwise next patch. |
|
@aequitas Indeed, there is already such an XML lib as seen in yamlgraph2/ being used. |
|
@purpleidea I don't see any xml reference in yamlgraph2? Can you point me to where it is? |
|
@aequitas Sorry, by brain accidentally some letters. Yaml*: https://github.com/purpleidea/mgmt/blob/master/yamlgraph2/gconfig.go#L31 |
69d190a to
281e7b0
Compare
|
@purpleidea so you want xml output or yaml output? :) Why not both? We could maybe use YAML as an intermediate format for XML (and JSON for that matter :D) |
|
@aequitas Just yaml is sufficient :) |
| "github.com/urfave/cli" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
https://golang.org/doc/effective_go.html#mixed-caps
probably these could be public if you want.
There was a problem hiding this comment.
i know, i'm following the names set by the tabwriter package, let me know if you prefer proper go names instead
There was a problem hiding this comment.
oh gosh. Another inconsistency in their own style guide? Gah. Sorry. Do the right thing. If golangv2 fixes these things, we'll be ahead of the curve.
8144f05 to
c855607
Compare
For now just lists resources and functions, more to be added soon.