-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue delete and insert edits when changing parameters #62897
Conversation
@tmat PTAL |
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
Show resolved
Hide resolved
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
Show resolved
Hide resolved
|
||
// Some sanity checks | ||
if (otherModel is null || | ||
containingSymbol.DeclaringSyntaxReferences.Length != 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part wasn't a problem with partial methods, so maybe I missed something, but I added a test, and another check to make the test pass :)
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
Outdated
Show resolved
Hide resolved
} | ||
|
||
// Now we can work out which is the old and which is the new, depending on which map we found | ||
// the match in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the caller already know this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The caller knows which map the lookup will work in, yes.
src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs
Outdated
Show resolved
Hide resolved
Reviewed 5 commits. Looks good, just a few minor things. |
ping @tmat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of #59264
I was going to do return type changes with this too, but need to either change SymbolKey, or changes to the SemanticEditInfoComparer: https://github.com/dotnet/roslyn/blob/main/src/Features/Core/Portable/EditAndContinue/AbstractEditAndContinueAnalyzer.cs#L3416