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.

Sign up for GitHub

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

Jump to bottom

Don't report rude edits for using declaration changes #51977

Merged
merged 9 commits into from
Mar 26, 2021
Merged

Don't report rude edits for using declaration changes #51977

merged 9 commits into from
Mar 26, 2021

Conversation

davidwengier
Copy link
Member

Fixes #10043

This turned out really straight forward. Let me know there are more tests to cover.

@davidwengier davidwengier requested review from a team as code owners March 19, 2021 05:43
@davidwengier
Copy link
Member Author

@tmat in case you hadn't seen this

tmat
tmat approved these changes Mar 25, 2021
View reviewed changes
Copy link
Member

@tmat tmat 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.

Looks good modulo the ifdef in the test.

Let's also make the same change for VB - can be a separate PR.

Let's file an issue to follow up on in future (if necessary) to detect semantic changes caused by changed using directives and report warnings on them.

davidwengier
davidwengier commented Mar 26, 2021
View reviewed changes
src/EditorFeatures/Core/Implementation/IntelliSense/AsyncCompletion/CompletionSource.cs
@@ -267,8 +267,6 @@ static void CheckForExperimentStatus(ITextView textView, Document document)
roslynTrigger = new CompletionTrigger(CompletionTriggerKind.Snippets);
}

var disallowAddingImports = _isDebuggerTextView ||
Copy link
Member Author

@davidwengier davidwengier Mar 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@genlu for completion changes.

This doesn't actually fix the issue, but at least the items show up in completion. Somehow the session properties still have DisallowAddingImports set to true, but I haven't been able to work out where that comes from yet. I set breakpoints everywhere that its set to true, and they don't get hit ¯_(ツ)_/¯

Not going to block this PR on that though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nevermind, cleaned out my hive and this does fix it. Caches be caching I guess? ¯\_(ツ)_/¯

@davidwengier
Copy link
Member Author

davidwengier commented Mar 26, 2021 •
edited
Loading

Proof! (before I fixed completion)

EnCAddImport

@davidwengier davidwengier mentioned this pull request Mar 26, 2021
Open
@davidwengier
Copy link
Member Author

Let's file an issue to follow up on in future (if necessary)

#52161

@davidwengier
Copy link
Member Author

Gonna merge this because it's Friday for me, and we can sneak it into P2. Gen I'll follow up with any completion changes you want.

@davidwengier davidwengier merged commit 5499541 into dotnet:main Mar 26, 2021
@davidwengier davidwengier deleted the EnCUsings branch March 26, 2021 07:01
@ghost ghost added this to the Next milestone Mar 26, 2021
@allisonchou allisonchou modified the milestones: Next, 16.10.P2 Mar 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Youssef1313 Youssef1313 Youssef1313 left review comments

@tmat tmat tmat approved these changes

Assignees
No one assigned
Labels
Area-IDE
Projects
None yet
Milestone
16.10.P2
Development

Successfully merging this pull request may close these issues.

Enc: Adding a namespace import shouldn't be a rude edit
4 participants
@davidwengier @tmat @Youssef1313 @allisonchou