Skip to content
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

Merged
merged 7 commits into from
Aug 2, 2022

Conversation

davidwengier
Copy link
Contributor

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

@davidwengier
Copy link
Contributor Author

@tmat PTAL

@davidwengier davidwengier changed the title Issue delete and insert edits when changing parameter types Issue delete and insert edits when parameters Jul 26, 2022
@davidwengier davidwengier changed the title Issue delete and insert edits when parameters Issue delete and insert edits when changing parameters Jul 26, 2022

// Some sanity checks
if (otherModel is null ||
containingSymbol.DeclaringSyntaxReferences.Length != 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

containingSymbol.DeclaringSyntaxReferences.Length != 1

Does this exclude partial methods? Doc please.

Copy link
Contributor Author

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 :)

}

// Now we can work out which is the old and which is the new, depending on which map we found
// the match in
Copy link
Member

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?

Copy link
Contributor Author

@davidwengier davidwengier Jul 27, 2022

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.

@tmat
Copy link
Member

tmat commented Jul 26, 2022

Reviewed 5 commits. Looks good, just a few minor things.

@davidwengier
Copy link
Contributor Author

ping @tmat

Copy link
Member

@tmat tmat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@davidwengier davidwengier merged commit 479fdd4 into dotnet:main Aug 2, 2022
@davidwengier davidwengier deleted the EnCSignatureChanges branch August 2, 2022 22:09
@ghost ghost added this to the Next milestone Aug 2, 2022
@dibarbet dibarbet removed this from the Next milestone Sep 1, 2022
@dibarbet dibarbet added this to the 17.4 P2 milestone Sep 1, 2022
@tmat tmat added this to the 17.4 milestone May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants