Implement textDocument/codeAction #252
Conversation
felixfbecker
force-pushed
the
code-actions
branch
2 times, most recently
from
207cb20 to
3ed3f6c
Compare
Codecov Report
@@ Coverage Diff @@
## master #252 +/- ##
==========================================
+ Coverage 58.16% 58.25% +0.08%
==========================================
Files 13 13
Lines 2087 2132 +45
Branches 345 352 +7
==========================================
+ Hits 1214 1242 +28
- Misses 734 744 +10
- Partials 139 146 +7
Continue to review full report at Codecov.
|
felixfbecker
changed the title
felixfbecker
force-pushed
the
code-actions
branch
from
3ed3f6c to
53be558
Compare
|
Rebased, @tomv564 does this work for you? |
| }); | ||
| } as any); | ||
| } as any); |
| * execution on the server. In most cases the server creates a WorkspaceEdit structure and | ||
| * applies the changes to the workspace using the request workspace/applyEdit which is sent from | ||
| * the server to the client. | ||
| */ |
There was a problem hiding this comment.
Great to see some documentation on how this works!
| end: ts.getLineAndCharacterOfPosition(sourceFile, span.start + span.length) | ||
| }, | ||
| newText | ||
| })); |
There was a problem hiding this comment.
So that's where all of convertTextChange went. Nice :)
| if (!params.arguments || params.arguments.length < 1) { | ||
| return Observable.throw(new Error(`Command ${params.command} requires arguments`)); | ||
| } | ||
| return this.executeCodeFixCommand(params.arguments, span); |
There was a problem hiding this comment.
I forgot to highlight this earlier in the PR, but throwing all of arguments into this.executeCodeFixCommand(... feels a bit dirty. Perhaps the "proper" way is to make sure the FileTextChanges array itself is in params.arguments[0]. What do you think?
There was a problem hiding this comment.
Why do you think it's dirty?
There was a problem hiding this comment.
What if in the future - besides the array of FileTextChanges - we also need to pass some Context c from the CodeAction to executeCodeFixCommand.
If the FileTextChanges live in params.arguments[0], then params.arguments[1] would be the logical place to put this Context parameter.
If we keep the FileTextChanges in all of params.arguments, then where would this extra Context parameter go?
There was a problem hiding this comment.
I would prefer the first parameter to be an object { context, fileTextChanges } then.
There was a problem hiding this comment.
Yes, that sounds good!
I guess you are free to modify codeFix's use of Command arguments at any time without compatibility issues, so there is no need to change anything now.
|
I just checked out and built this branch and verified both Rename and the codeFix Code Action work |
#251 to be merged first
@tomv564