Hoffs / omnisharp-extended-lsp.nvim

Extended 'textDocument/definition' handler for OmniSharp Neovim LSP (now also `textDocument/references`, `textDocument/implementation` and source generated files)
147 stars 18 forks source link

feat(type-definition): add support for type-definition #33

Closed tcrundall closed 7 months ago

tcrundall commented 7 months ago

Implements jump to type definition, useful for objects with types from third party libraries. (addresses issue #26 )

Refactors error message in definition to extract method name from context, thereby allowing reuse of method for both definition and typeDefinition.

(This is my first PR to open source code, so any feedback is very welcome)

Hoffs commented 7 months ago

Hi,

Thanks for the contribution, looks good overall!

I am just not really sure about putting 2 different commands into one file, it was intentionally split so that each command would be contained within a different file. From the commits, I see you did it that way at first as well :)

While it's certainly code duplication, I think in this case it's acceptable, as they are different commands and could potentially have different implementations and requirements, just as it stands, they do not. Even Omnisharp source has distinct request/response types and handler implementation that are similar yet are completely separate types.

What do you think? It could be merged as-is as well and split if there is ever a need down the road.

tcrundall commented 7 months ago

Thanks!

Yeah I wasn't too sure tbh and figured I'd let you decide your preferred structure. It seemed too good an opportunity to reuse the same methods, but I agree that separate files per command looks cleaner.

I'll do as you suggest and split it back into two files. If any more commands get added in the future maybe the common methods could get refactored out.

tcrundall commented 7 months ago

done :)

Hoffs commented 7 months ago

Thanks!