graph/encoding/graphql: add new package for decoding GraphQL output #169
Conversation
There was a problem hiding this comment.
LGMT. Happy to see that you are providing support for more input and output formats for the graph representation @kortschak. I don't have experience with graphql since before, so the review comments are more about the general code style rather than specifics related to graphql.
graph/encoding/graphql/decode.go
Outdated
| // uidName is the name of the UID field in the source JSON. | ||
| uidName string | ||
|
|
||
| nodes map[string]graph.Node |
There was a problem hiding this comment.
Add a comment stating the key of the map; e.g.
// nodes maps from node value to graph node.Is this comment correct? Are node values unique?
There was a problem hiding this comment.
Done.
UIDs are unique in the input text.
| if err != nil { | ||
| return err | ||
| } | ||
| value := fmt.Sprint(v) |
There was a problem hiding this comment.
What does the call to fmt.Sprint do? Can it be replaced with a type assertion for fmt.Stringer?
There was a problem hiding this comment.
Everything can be represented as a string, fmt.Sprint(v) does this conversion. The two things we are likely to get here are a string or an int. Neither of these implement fmt.Stringer.
| return err | ||
| } | ||
| if attr == g.uidName { | ||
| value := fmt.Sprint(v) |
|
PTAL |
|
LGTM |
|
Can you re-approve please. |
|
I tried to have a look around, but can't find the option to do so in the web UI. |
|
It says
and now, the review button is gone for what it looks like. |
|
I think the reason may be the force push,
|
|
You need to be in the files changed tab, where you'll see green "Review changes" button/menu/dialogue. The review is dismissed by any additional push. |
Ok, I'll take a look. |
|
For future notes, @kortschak Did you do a force push, or how did you push the latest incremental commit? The benefit of uploading incremental commits, is that it is easier to review the changes, then you see the exact lines which change. Also, when the PR is complete, you may hit squash and merge to turn those several commits into a single one. |
|
It was forced. In this PR there was very minimal change (the changes are visible from the GUI - it's unfortunate githubs git GC is so aggressive in removing those commits - it did not use to be) in addressing the comments - and significantly, the PR would not pass unless rebased onto the changes in #168 merged into master. Normally I do do incremental and retain the history (at least since the addition of the squash option on the GUI). |
|
I see. Thanks for the background. |
Please take a look.
Depends on #168.
Closes #72.