add fillstruct tool #1506
add fillstruct tool #1506
Conversation
There was a problem hiding this comment.
@buyology I took the liberty of refactoring a bit.
I have left 2 comments. Please do take a look and add the tests. I hope to have this in the next update
Great job!
src/goFillStruct.ts
Outdated
| code: string; | ||
| } | ||
|
|
||
| export function fillStruct() { |
There was a problem hiding this comment.
Any reason to export this function when we already have the runFillStruct?
src/goFillStruct.ts
Outdated
| args.push(offset.toString()); | ||
| } else if (editor.selection.start.line <= editor.selection.end.line) { | ||
| args.push('-line'); | ||
| args.push(`${editor.selection.start.line + 1},${editor.selection.end.line + 1}`); |
There was a problem hiding this comment.
When the command is run on a selection, fillstruct fails with invalid value "..." for flag -line: strconv.ParseInt: parsing "...": invalid syntax
Looks like the -line flag takes a single number and we are passing 2 here.
Also, in what scenario do we expect the user select text and then run the command?
There was a problem hiding this comment.
I had that change sitting on my computer :) Since it takes a -line, I thought we might as well support selecting a whole line.
src/goMain.ts
Outdated
| @@ -434,6 +440,7 @@ function sendTelemetryEventForConfig(goConfig: vscode.WorkspaceConfiguration) { | |||
| includeImports: goConfig['gotoSymbol'] && goConfig['gotoSymbol']['includeImports'] + '', | |||
| addTags: JSON.stringify(goConfig['addTags']), | |||
| removeTags: JSON.stringify(goConfig['removeTags']), | |||
| fillStruct: JSON.stringify(goConfig['fillStruct']), | |||
There was a problem hiding this comment.
This is only for configuration. Since there is no config called fillStruct, this change is not needed
|
Thanks for your comments. Added the tests. However — after the refactoring I'm back at that same problem that we discussed on Slack that I can see during the tests that stuff is properly edited, but the tests fail. I cannot spot the obvious missing return in this case however. |
|
Sorry about that, fixed the issue with the tests. |
|
Nice! :) |
|
Will just wait for the tests to complete and then merge |
Change-Id: Iee9b0230618f13e9c864dfee03456c0f8f2a5993 Signed-off-by: nickboldt <nboldt@redhat.com>
Closes #1227
This adds the nice
fillstructtool.