Fixes bugs when there are multiple handlers are defined for a language for a given request

[dmgonch]

OmniSharp supports plugins via “--plugin” command like option. A plugin may expose handlers for operations like “Go to definition” or “Find Symbols”. These changes fix bugs in how handers are invoked in this case and how their results are merged. Unit tests added demonstrate which scenarios are fixed with these changes.

[dmgonch]

@david-driscoll, @filipw appreciate your guys feedback #Closed

[filipw]

thanks a lot, while I think I understand technically what is going on here, could you provide a real world example of a situation this addresses? it would be helpful 😀

[dmgonch]

thanks a lot, while I think I understand technically what is going on here, could you provide a real world example of a situation this addresses? it would be helpful 😀

Take a look at #1383. I'm thinking of a new extension that plugs into OmniSharp and provides additional implementation for few services to enable code navigation while projects are still being loaded (or their load is done lazily when omnisharp.enableMsBuildLoadProjectsOnDemand setting is set). The plugin will inspect Roslyn workspace and provide symbol information only for projects that haven't been loaded yet thus achieving "smart" merging of results. #Closed

[dmgonch]

@filipw Please let me know if you have any concerns about the change or have a better idea how the scenarios I outlined can be enabled. #Closed

[dmgonch]

@david-driscoll, @filipw curious what you guys think about the proposed fix #Closed

[filipw]

I don't have anything against, I just worry about adding additional overhead, other than that looks OK to me.
I'll suggest @david-driscoll has a look as he initially bootstrapped that part of the code

[dmgonch]

@filipw @david-driscoll Please advise if the last update still leaves some feedback concerns not covered.

[filipw]

LGTM - thanks

[dmgonch]

@filipw Thanks! Please advise what to do with AppVeyor's check failure above since it looks like the latest master was successfully merged into this branch by you.

[filipw]

yeah looks like the merge of latest master didn't trigger the build correctly, can you try to rebase this on top of master and force push instead?

[dmgonch]

@filipw All checks have passed! ;)

[dmgonch]

@filipw @david-driscoll Folks, please advise if anything is needed on my side before these changes can be merged.

[filipw]

nothing - looks good to me 👍

[dmgonch]

@filipw @david-driscoll Unless you guys see anything else that needs to be done for this change, I appreciate if you guys could merge it into the master.

[dmgonch]

@filipw @david-driscoll Appreciate if you guys could comment.

[filipw]

thanks, as I said, it looks good to me and I gave my approval already 😀
however, I'd like @david-driscoll to have a second look since he wrote that code originally - hope this makes sense

[dmgonch]

@david-driscoll Do you see anything else that needs to be done for this change? Thanks!

[dmgonch]

@david-driscoll Appreciate if you could comment. Thanks!

[dmgonch]

@filipw Do you think you will be comfortable with pulling these changes into master? @david-driscoll I hope you don't mind - but I would like to move forward with these changes unless you see more issues to address. Appreciate your help folks.

[david-driscoll]

@dmgonch sorry for the delay I just saw something ✨ shiny ✨ . Updating the branch and will check back in today. It looks like all my concerns were hit.

[david-driscoll]

@dmgonch Thanks for the contribution! Sorry for the delay however. 💯