Skip to content

Ensure that we show all applicable refactorings for lightbulb under t…#44796

Merged
mavasani merged 1 commit intodotnet:masterfrom
mavasani:Issue44594
Jun 4, 2020
Merged

Ensure that we show all applicable refactorings for lightbulb under t…#44796
mavasani merged 1 commit intodotnet:masterfrom
mavasani:Issue44594

Conversation

@mavasani
Copy link
Contributor

@mavasani mavasani commented Jun 2, 2020

…he caret

Fixes #44594

Verified that rename tracking code action now shows up in the lightbulb under the caret

@mavasani mavasani added this to the 16.7.P3 milestone Jun 2, 2020
@mavasani mavasani requested review from a team, CyrusNajmabadi and sharwell June 2, 2020 18:59

var filteredRefactorings = FilterOnUIThread(refactorings, workspace);
// If we are computing refactorings outside the 'Refactoring' context, i.e. for example, from the lightbulb under a squiggle or selection,
// then we want to filter out refactorings outside the selection span.
Copy link
Contributor

Choose a reason for hiding this comment

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

i would explain 'why' (or link to an issue).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note in #44855

Comment on lines +311 to +313
return (new RenameTrackingCodeAction(
document, title, refactorNotifyServices, undoHistoryRegistry),
snapshotSpan.Span.ToTextSpan());
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return (new RenameTrackingCodeAction(
document, title, refactorNotifyServices, undoHistoryRegistry),
snapshotSpan.Span.ToTextSpan());
return (new RenameTrackingCodeAction(document, title, refactorNotifyServices, undoHistoryRegistry),
snapshotSpan.Span.ToTextSpan());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a note in #44855

@CyrusNajmabadi
Copy link
Contributor

Would really like an integration test here.

Copy link
Contributor

@CyrusNajmabadi CyrusNajmabadi left a comment

Choose a reason for hiding this comment

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

can we please do an integratin test here?

@mavasani
Copy link
Contributor Author

mavasani commented Jun 4, 2020

Would really like an integration test here.

I could not find an easy way to add an integration test here. Need to investigate how to invoke that lightbulb. I have filed #44855 to track this work, I'll prioritize it for today. However, I don't want to let this PR hang around longer and risk this regression shipping in 16.7, so I am going to merge this PR. Hope that is fine.

@CyrusNajmabadi
Copy link
Contributor

Hope that is fine.

Please do prioritize though. It is important we don't regress this.

I assume you did manually validate though?

@mavasani
Copy link
Contributor Author

mavasani commented Jun 4, 2020

I assume you did manually validate though?

Yes, I did!

Please do prioritize though. It is important we don't regress this

Yep, investigating now.

@mavasani mavasani merged commit c9ab85e into dotnet:master Jun 4, 2020
@ghost ghost modified the milestones: 16.7.P3, Next Jun 4, 2020
@mavasani mavasani deleted the Issue44594 branch June 4, 2020 18:27
@RikkiGibson RikkiGibson modified the milestones: Next, 16.7.P3 Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename object/variable names no longer available on the context menu

3 participants