Use the correct span to rename after invoking extract-method manually.#61369
Conversation
| using var undoTransaction = _undoManager.GetTextBufferUndoManager(subjectBuffer).TextBufferUndoHistory.CreateTransaction("Extract Method"); | ||
|
|
||
| // apply extract method code to buffer | ||
| var (document, _) = extractMethodResult.GetFormattedDocumentAsync(cleanupOptions, cancellationToken).WaitAndGetResult(cancellationToken); |
There was a problem hiding this comment.
the bug was here. we were throwing away the second part of the result of GetFormattedDocumentASync. That part of hte result tells us where the name token is to start inline-rename after the formatting occurs.
There was a problem hiding this comment.
Thanks for the explanation here :)
| // apply the change to buffer | ||
| // get method name token | ||
| ApplyChangesToBuffer(result, textBuffer, cleanupOptions, cancellationToken); | ||
| SyntaxToken methodNameAtInvocation; |
There was a problem hiding this comment.
Do we ever expect this to be a default value? From code it is hard to tell
There was a problem hiding this comment.
no. it can't ever be a default value. NOte: this is not assigned anything at the decl point. It's purely a declaration of the variable. there is one and only one value assignment to it from the method invoked.
There was a problem hiding this comment.
oh yea, I meant is it guaranteed to always be something or do we have cases where the returned value is wrong? Not blocking on this PR, just curious
There was a problem hiding this comment.
We weren't using it before, so I could see that this might be an area for a bug we don't know about :) Don't spend time on it, just if you know off hand.
ryzngard
left a comment
There was a problem hiding this comment.
Approving with assumption of incoming test
|
I'm just surprised and curious, why was this added without a test? Adding it later is something that can be easily forgotten. Also, I don't think the issue should be marked as closed until there's a test that verifies the behavior to prevent a regression. |
Because i'm working on tests as part of another PR in this area. |
Fixes #58971
Test incoming.