Fix FindImplementationService not finding all implementations of the partial class #1318
Conversation
|
can you add a test illustrating what this is fixing? |
|
Will push test in later today |
|
@filipw Sorry for the delay, I added test for finding all implementations of the partial class and one test for partial methods (to make sure this functionality is kept intact). |
|
I merged #1302, could you please resolve the conflicts - thanks! Otherwise, looks good to me |
# Conflicts: # src/OmniSharp.Roslyn.CSharp/Helpers/QuickFixExtensions.cs
|
@filipw done! |
| Assert.Single(implementations); | ||
|
|
||
| var implementation = implementations.First(); | ||
| Assert.Equal("SomeMethod", implementation.Name); |
There was a problem hiding this comment.
Assert on the location of implementation to prove you found the implementation copy?
There was a problem hiding this comment.
@SirIntruder if you could fix this feedback, this looks good to me.
| Assert.Single(implementations); | ||
|
|
||
| var implementation = implementations.First(); | ||
| Assert.Equal("SomeMethod", implementation.Name); |
There was a problem hiding this comment.
@SirIntruder if you could fix this feedback, this looks good to me.
|
@rchande @filipw @david-driscoll Thanks a lot for the tip, I would actually regress partial method without it. I enhanced the test and changed handler code, looks like it works correctly now.
|
|
looks good, indeed this is the behavior we want 👍 |
filipw
dismissed
david-driscoll’s stale review
requested changes were implemented
|
thanks! |
Currently, when you use "Find Implementations" on a partial class, it appears to randomly select one of the definitions instead of returning all of them (like VS does). Looks like this was unintentionally broken in order to handle partialMethod case (guess there is makes sense to take symbol.Locations.First() instead of symbol.Locations).
Should be easy to merge with @filipw fix for separate bug fix in the same service.