Skip to content
This repository has been archived by the owner on Apr 14, 2022. It is now read-only.

Go to definition behavior change request #1138

Closed
amittleider opened this issue May 29, 2019 · 5 comments · Fixed by #1343
Closed

Go to definition behavior change request #1138

amittleider opened this issue May 29, 2019 · 5 comments · Fixed by #1343
Assignees
Labels
enhancement New feature or request feature: symbols
Milestone

Comments

@amittleider
Copy link

Currently, using "Go to definition" with the MS Language Server will jump you to your import statement, likely at the top of your file, rather than jumping you into the class. I understand from a separate issue #850 that this is the intended behavior. Below is a gif of it.

58559204-dd9c0780-8222-11e9-9bed-cebbfa5b5504

In my view, "Go to definition" means that you go to where the class is defined. Taking me to the top of the file and showing me the import statement doesn't seem nearly as widely used or necessary as jumping directly to where the class is implemented.

Imagine you were in the full version of Visual Studio coding in C#, "Go to definition" would take you to the definition of your class.

If we insist that we need a separate feature to navigate us to the import statement, it seems that we should make a separate button and keyboard shortcut called "Go to declaration", and leave the "Go to definition" function to be the same behavior as Jedi.

@jakebailey
Copy link
Member

The LSP spec has both definition and declaration as of 3.14.0; perhaps we should implement both and have definition recurse until it hits the true assignment.

@MikhailArkhipov
Copy link

#494

@jahan01
Copy link

jahan01 commented Jun 5, 2019

this should be a bug, not the intended behaviour. Going by the name Go to definition should take to definition, not to the import.

And even if it is intended behaviour (then it should be called something else, IMO), it is not consistent, like in below situations

constants.py

def a_simple_function():
    pass

run.py

from vscode_test import constants
from vscode_test.constants import a_simple_function
constants.a_simple_function()
a_simple_function()

Goto definition of constants.a_simple_function() takes to constants.py directly where as a_simple_function takes to import and then to definition

@jakebailey @MikhailArkhipov

@MikhailArkhipov MikhailArkhipov self-assigned this Jun 5, 2019
@MikhailArkhipov MikhailArkhipov added this to the June 2019.1 milestone Jun 5, 2019
@MikhailArkhipov
Copy link

Sure. 'Bug' is when something intended to work certain way, but it doesn't. If something is implemented certain way intentionally, with the appropriate test, and it works as designed, it is not technically a bug. Sure, it could be a wrong choice though :-)

@jahan01
Copy link

jahan01 commented Jun 8, 2019

@MikhailArkhipov I don't know about language server implementation, but you also may to look at below issues together as it appears to me to originate from same design (if I am wrong, excuse me for making false noise)

  1. All references are not listed when using ms language server #1174, here not all references are identified (directly imported symbols are only considered for reference, it is not considered as referenced if used as <module>.<symbol>. Sounds like same inconsistency I spoke about in my earlier comment)
  2. import statements are listed as part of reference, though you can argue about this, jedi and other language server implementations (for other languages like java etc.,) doesn't list imports as reference. IMO, imports are not references.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request feature: symbols
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants