[RFC] Remap GoTo commands in Python completer #1200
Conversation
Map GoTo, GoToDefinition, and GoToDeclaration to Jedi goto_assignments. Add GoToType command and map it to Jedi goto_definitions.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @micbou)
ycmd/completers/python/python_completer.py, line 290 at r1 (raw file):
# set to True. Unfortunately, we can't use that option because it also # jumps to the original name of aliases which is unexpected as an alias is # a kind of definition. So, we need to traverse definitions by repeatedly
Hmmmmmmm.
I have personally become quite used to the behaviour of jumping to the import statement.
In fact, I rely on it. Often I will be looking at some code with the goal of, let's say, "re-appropriating" it for something. I see a type or something that I need to use, and do my GoTo mapping on it to get the import statement that I also need.
If I happen to actually want the real def, I just hit the mapping 2x.
So changing this is tricky in the sense that there's a real use case for the current behaviour, and probably why it's an option in Jedi's API.
I realise it is perhaps somewhat arbitrary, but maybe GoToDefinition jumps through imports, but GoToDeclaration jumps to imports ? Not sure.
Thoughts?
Codecov Report
@@ Coverage Diff @@
## master #1200 +/- ##
==========================================
- Coverage 97.28% 97.09% -0.19%
==========================================
Files 95 95
Lines 7109 7128 +19
==========================================
+ Hits 6916 6921 +5
- Misses 193 207 +14 |
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
ycmd/completers/python/python_completer.py, line 290 at r1 (raw file):
Previously, puremourning (Ben Jackson) wrote…
Hmmmmmmm.
I have personally become quite used to the behaviour of jumping to the import statement.
In fact, I rely on it. Often I will be looking at some code with the goal of, let's say, "re-appropriating" it for something. I see a type or something that I need to use, and do my GoTo mapping on it to get the import statement that I also need.
If I happen to actually want the real def, I just hit the mapping 2x.
So changing this is tricky in the sense that there's a real use case for the current behaviour, and probably why it's an option in Jedi's API.
I realise it is perhaps somewhat arbitrary, but maybe
GoToDefinitionjumps through imports, butGoToDeclarationjumps to imports ? Not sure.Thoughts?
Sorry for taking a month to respond to this one... I completely forgot about it after our brief talk in gitter.
I'm not sure what @puremourning is referring to as "re-appropriating", but I too have had a need to see from where was something imported. Though I mostly used * followed by gg in vim. Much more often, my intention for :YcmCompleter GoTo was to jump through the imports and to the actual definition.
There was a problem hiding this comment.
Reviewed 12 of 12 files at r1.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
ycmd/completers/python/python_completer.py, line 290 at r1 (raw file):
Previously, bstaletic (Boris Staletic) wrote…
Sorry for taking a month to respond to this one... I completely forgot about it after our brief talk in gitter.
I'm not sure what @puremourning is referring to as "re-appropriating", but I too have had a need to see from where was something imported. Though I mostly used
*followed byggin vim. Much more often, my intention for:YcmCompleter GoTowas to jump through the imports and to the actual definition.
Using GoTo to find how a symbol is imported doesn't seem right to me. What about displaying that information with the GetType command? Jedi returns the fully qualified name in the full_name field. For instance, instead of returning the following info on GetType for the ExpandVariablesInPath function imported in that file:
def ExpandVariablesInPath(path)we could return
def ycmd.utils.ExpandVariablesInPath(path)which is enough to know how to import this symbol.
There was a problem hiding this comment.
Reviewable status: 0 of 2 LGTMs obtained (waiting on @puremourning)
ycmd/completers/python/python_completer.py, line 290 at r1 (raw file):
Previously, micbou wrote…
Using
GoToto find how a symbol is imported doesn't seem right to me. What about displaying that information with theGetTypecommand? Jedi returns the fully qualified name in thefull_namefield. For instance, instead of returning the following info onGetTypefor theExpandVariablesInPathfunction imported in that file:def ExpandVariablesInPath(path)we could return
def ycmd.utils.ExpandVariablesInPath(path)which is enough to know how to import this symbol.
That sounds good to me.
|
Submitted #1275 |
The
GoToDefinition,GoToDeclaration, andGoTocommands in the Python completer are currently mapped to Jedi methods as follows:GoToDefinitiontogoto_definitions;GoToDeclarationtogoto_assignments;GoTotogoto_definitionsthengoto_assignmentsifgoto_definitionsfailed.These mappings don't really make sense since there is no distinction between a definition and a declaration in Python. All these
GoTocommands should be identical. Said that, we need to decide to which function these commands should be mapped:goto_definitionsorgoto_assignments? My understanding is thatgoto_definitionsdoesn't jump to the definition but the type definition whilegoto_assignmentsactually jumps to the definition with the caveat that it doesn't follow imports by default. This is confirmed by looking at some examples (these examples are used in the tests):With the cursor on the second
my_variable:goto_definitionsreturns theintbuiltin. This is clearly not the definition ofmy_variablebut its type;goto_assignmentsreturns the firstmy_variablewhich is wheremy_variableis defined.With the cursor on the second
my_instance:goto_definitionsreturns the firstMyClass. Again, not the definition ofmy_instancebut its type;goto_assignmentsreturns the firstmy_instance.This PR thus proposes to remap the three
GoTocommands togoto_assignments(with some additional work to follow imports and to deal with aliases) and to introduce a new commandGoToTypewhich is mapped togoto_definitions.This change is