Skip to content

Have inheritance-margin hold onto data that does *not* point back at Solution instances.#55696

Merged
CyrusNajmabadi merged 18 commits intodotnet:mainfrom
CyrusNajmabadi:inheritanceMarginMemory
Aug 20, 2021
Merged

Have inheritance-margin hold onto data that does *not* point back at Solution instances.#55696
CyrusNajmabadi merged 18 commits intodotnet:mainfrom
CyrusNajmabadi:inheritanceMarginMemory

Conversation

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor

@CyrusNajmabadi CyrusNajmabadi commented Aug 18, 2021 •
edited by Pilchie
Loading

Fixes AB#1371724

@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner August 18, 2021 16:22
@ghost ghost added the Area-IDE label Aug 18, 2021
@CyrusNajmabadi CyrusNajmabadi mentioned this pull request Aug 18, 2021
Merged
@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented Aug 18, 2021

@sharwell @JoeRobich can you ptal? tahnks!

JoeRobich
JoeRobich approved these changes Aug 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

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

I could see there being tests around the Detach()/ReHydrate(), if only because of the omission we caught.

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented Aug 18, 2021

I could see there being tests around the Detach()/ReHydrate(), if only because of the omission we caught.

@Cosifne do we have integration tests for inheritance margin? if not, can we file an issue to add them asap?

@Cosifne
Copy link
Copy Markdown
Member

Cosifne commented Aug 18, 2021

I could see there being tests around the Detach()/ReHydrate(), if only because of the omission we caught.

@Cosifne do we have integration tests for inheritance margin? if not, can we file an issue to add them asap?

There are only unit tests in for inheritance Margin.
Sure I can file a issue for that

@CyrusNajmabadi
Copy link
Copy Markdown
Contributor Author

CyrusNajmabadi commented Aug 18, 2021

There are only unit tests in for inheritance Margin.
Sure I can file a issue for that

Thanks. It's always good to have at least one integration test to totally validate the end to end, (esp. with custom UI).

Cosifne
Cosifne approved these changes Aug 18, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@Cosifne Cosifne left a comment •
edited
Loading

Choose a reason for hiding this comment

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

One small problem in the InheritanceMargin.xaml.cs, other than LGTM

CyrusNajmabadi
CyrusNajmabadi commented Aug 19, 2021
View reviewed changes
src/EditorFeatures/Core/GoToDefinition/GoToDefinitionHelpers.cs
CancellationToken cancellationToken)
{
if (definitions.IsDefaultOrEmpty)
return false;
Copy link
Copy Markdown
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Aug 19, 2021

Choose a reason for hiding this comment

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

this moved inside TryNavigateToOrPresentItemsAsync

@CyrusNajmabadi CyrusNajmabadi requested review from a team as code owners August 19, 2021 21:56
@333fred 333fred disabled auto-merge August 19, 2021 22:53
@333fred
Copy link
Copy Markdown
Member

333fred commented Aug 19, 2021 •
edited
Loading

@CyrusNajmabadi it looks like there's some merge commits in here that are likely bringing in code that was not originally reviewed (as it's now requesting compiler reviews), so I disabled automerge so you can make sure everything is kosher.

@CyrusNajmabadi CyrusNajmabadi merged commit 6ce4dc3 into dotnet:main Aug 20, 2021
@ghost ghost added this to the Next milestone Aug 20, 2021
@CyrusNajmabadi CyrusNajmabadi deleted the inheritanceMarginMemory branch August 20, 2021 04:50
@dibarbet dibarbet modified the milestones: Next, 17.0.P4 Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoeRobich JoeRobich JoeRobich approved these changes

@sharwell sharwell Awaiting requested review from sharwell

+1 more reviewer

@Cosifne Cosifne Cosifne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area-IDE

Projects

None yet

Milestone

17.0.P4

Development

Successfully merging this pull request may close these issues.

6 participants

@CyrusNajmabadi @Cosifne @333fred @JoeRobich @sharwell @dibarbet