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

Multiple Definitions Returned #449

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Multiple Definitions Returned #449

wants to merge 6 commits into from

Conversation

thchha
Copy link
Contributor

@thchha thchha commented Aug 12, 2022

Hey Mr. Bosch,

thank you for your efforts.
I am using your lsp-client implementation as a reference for my own language server since it is the most complaint one. Your code looks great. :)

This pull request requests your opinion on the prompted change.
I would appreciate mentoring in adapting the pull request to your needs, if the essence is acceptable.

Sincerly,
Thomas

Thomas added 4 commits August 12, 2022 21:02
If the result is an list with more then one result, prompt the user the
71181e0 
quickfixlist with the options to select.
Selecting an option should be in terms of the replicated vim behaviour
(my assumption).

This _could_ be useful, if the langauge is lexical scoped.
There should be only _one declaration_.
Multiple _re_definitions could appear.
necessary documentation updates.
11c7bf2
do not execute, but call the function to open the quickfix list instead.
67e4d1a
the PR natebosch#449 requests the ability to handle multiple definiti…
d572647 
…ons.

This branch is concerned with compability.
It should:
* enable jumping to (1) declaration => with split, only one binding
* extend the existing documentation => reference parent impl
* extend the existing tests => copy dart-test
@thchha
Copy link
Contributor Author

thchha commented Aug 12, 2022 •
edited
Loading

I created a secondary branch, which introduces the old behaviour, but with the request of textDocument/declaration. You can find (and merge/refer to it without obligations) to it at https://github.com/thchha/vim-lsc/tree/declaration .

The user behaviour changes for the following:

When one binding is provided, both bindings (C-W or just C-]) is working as expected.
When there are multiple bindings, both bindings will call copen().

The complementary branch will enable the previous behavior on a differing binding called gd, which is mnemonic and free (and requests textDocument/declaration).

note that textDocument/declaration can return multiple bindings as well; But I agree that this should not be the case.

Thomas Hage and others added 2 commits August 12, 2022 23:03
documentation should refer to the default binding.
5c26752
@thchha
If the length of the result contains multiple items, prompt copen.
1ee6df6 
If there is only a single item in an array, diretly chose this.
If the result isn't an array, we can use it similiarly.

Somehow, I incremented this check.
This fixes it.
thchha pushed a commit to thchha/vim-lsc that referenced this pull request Dec 7, 2022
@thchha
the PR natebosch#449 requests the ability to handle multiple definiti…
0d6a0c2 
…ons.

This branch is concerned with compability.
It should:
* enable jumping to (1) declaration => with split, only one binding
* extend the existing documentation => reference parent impl
* extend the existing tests => copy dart-test
@thchha
Copy link
Contributor Author

thchha commented Dec 7, 2022

@natebosch You can test the behaviour of my PR in unity at the following branch: https://github.com/thchha/vim-lsc/tree/thchha

Please be aware that I am new to contributing. Just pinpoint the issues you have in your gut.
I will dedicate these and adapt, until it proves the quality to be squashed.

thchha pushed a commit to thchha/vim-lsc that referenced this pull request Dec 7, 2022
@thchha
the PR natebosch#449 requests the ability to handle multiple definiti…
5a983a1 
…ons.

This branch is concerned with compability.
It should:
* enable jumping to (1) declaration => with split, only one binding
* extend the existing documentation => reference parent impl
* extend the existing tests => copy dart-test
kohnish added a commit to kohnish/vim-lsc that referenced this pull request Mar 3, 2023
@kohnish
copen was broken
d641260 
(Multiple Definitions Returned natebosch#449)
natebosch#449
71181e0
natebosch
natebosch reviewed Mar 25, 2023
View reviewed changes
autoload/lsc/config.vim
@@ -3,6 +3,8 @@ if !exists('s:initialized')
let s:default_maps = {
\ 'GoToDefinition': '<C-]>',
\ 'GoToDefinitionSplit': ['<C-W>]', '<C-W><C-]>'],
\ 'GoToDeclaration': 'gd',
\ 'GoToDeclarationSplit': ['<C-W>]', '<C-W>gd'],
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we can map '<C-W>]' again since it's mapped for GoToDefinitionSplit

Suggested change
\ 'GoToDeclarationSplit': ['<C-W>]', '<C-W>gd'],
\ 'GoToDeclarationSplit': ['<C-W>]', '<C-W>gd'],

doc/lsc.txt
@@ -75,6 +76,15 @@ will default to opening a vertical split, while >
<
will prefer a new tab.

*:LSClientGoToDeclaration*
Similiar to |LSClientGoToDefinition| but expects only a single result when
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we should treat declarations specially since they allow multiple too.

What do you think about always jumping to the first element in the list for either case, but also populating the quickfix list with the extras when there are more?

kohnish pushed a commit to kohnish/vim-lsc that referenced this pull request Mar 27, 2023
@kohnish
the PR natebosch#449 requests the ability to handle multiple definiti…
46d3564 
…ons.

This branch is concerned with compability.
It should:
* enable jumping to (1) declaration => with split, only one binding
* extend the existing documentation => reference parent impl
* extend the existing tests => copy dart-test
kohnish added a commit to kohnish/vim-lsc that referenced this pull request Mar 27, 2023
@kohnish
copen was broken
a48fb8a 
(Multiple Definitions Returned natebosch#449)
natebosch#449
71181e0
kohnish added a commit to kohnish/vim-lsc that referenced this pull request Jun 30, 2023
@kohnish
Revert "the PR natebosch#449 requests the ability to handle multiple …
2a1b853 
…definitions."

This reverts commit 46d3564.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natebosch natebosch natebosch left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thchha @natebosch