-
Notifications
You must be signed in to change notification settings - Fork 420
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
when fixing usings, return namespaces associated with ambiguous result #1169
Conversation
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.
Thanks for the PR!
I'm having trouble understanding the scenario. Can you
add applicable test(s) so that we don't regress this, and to help me understand what's being added?
Hi Ravi,
I did add a test addressing the scenario in question.
I use omnisharp-vim with omnisharp-roslyn as a dependency. When I run the fix usings command, sometimes I get a response indicating there is an ambiguous reference. This PR would update the message that comes back to not only indicate an ambiguous reference, but also indicate the ambiguous namespaces. That way, I as the user can decide which reference I want to use.
If you take a look at the commits you should see a test addressing this scenario.
Thanks,
Tolu
…________________________________
From: Ravi Chande <notifications@github.com>
Sent: Monday, April 30, 2018 9:24:19 AM
To: OmniSharp/omnisharp-roslyn
Cc: Tolu Adesegha; Author
Subject: Re: [OmniSharp/omnisharp-roslyn] when fixing usings, return namespaces associated with ambiguous result (#1169)
@rchande requested changes on this pull request.
Thanks for the PR!
I'm having trouble understanding the scenario. Can you
add applicable test(s) so that we don't regress this, and to help me understand what's being added?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub<#1169 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AIW3CFFoNUVNvc5i95Uj-Mvs4-poZrDzks5ttyyjgaJpZM4Tex8W>.
|
@tadesegha I'm sorry, you did add a test. I missed it since it was at the bottom of the diff. I do think there are some issues with the test you modified. Mind taking a look? I'm happy to help out if you have any questions. |
@rchande the last line of the test 'await AssertUnresolvedReferencesAsync(content.Code, expectedUnresolved);' has assertions. If you update the test without updating the code you should get a failing test. |
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.
LGTM - looks good
the potential using statements are added to the Text of the QuickFix response, allowing the user manually choose which using statement to employ.