[WIP] Add RefactorRename subcommand to Python completer

[micbou]

Heavily inspired by PR #417.

Jedi does not return the end location of a definition so we find it by computing its name length.

Update JediHTTP submodule to include PR vheon/JediHTTP#13.

Refactor the subcommands Python tests to reduce code duplication.

---

This change is

[Valloric]

Thanks!

This seems to me, though @puremourning should probably take a look as well since he wrote #417.

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[puremourning]

YAY \o/

This is very cool.

Though I think there is some overlap with #417 (around the test matchers). So they might conflict. Other than that 👍

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[homu]

☔ The latest upstream changes (presumably #417) made this pull request unmergeable. Please resolve the merge conflicts.

[puremourning]

I did give this a go, and FWIW it is probably worth noting the limitations of this (something I also need to do for the javascript and typescript completers): Jedi can only see modules that are imported.

With javascript and typescript completers there are "project" files which, when configured, tell the semantic engine about files it would't normally be able see. I worry that this limits the effectiveness of python renames, in the sense that it is potentially incomplete.

Here's a trivial example:

---

Reviewed 9 of 9 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

---

Comments from the review on Reviewable.io

[vheon]

I'm not sure I would ship a "half" feature to the user, but is true that jedi-vim do exactly this IIRC so at least we wouldn't be worse but still...

[Valloric]

It's concerning, I agree. Thoughts, everyone? Which of you would/wouldn't like to ship this, and why? I'm on the fence.

[micbou]

Fixed the conflicts.

With javascript and typescript completers there are "project" files which, when configured, tell the semantic engine about files it would't normally be able see.

This can be done by adding files to jedi.settings.additional_dynamic_modules. We would need to implement a strategy to find the files and to pass them to this variable. This would involve changes in JediHTTP. I have some ideas on how to do this but for another PR.

I'm not sure I would ship a "half" feature to the user, but is true that jedi-vim do exactly this IIRC so at least we wouldn't be worse but still...

This is already affecting the GoToReferences subcommand so I don't see why we would ship GoToReferences but not RefactorRename (both commands are using the usages request).

---

Reviewed 2 of 4 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[vheon]

This can be done by adding files to jedi.settings.additional_dynamic_modules. We would need to implement a strategy to find the files and to pass them to this variable. This would involve changes in JediHTTP. I have some ideas on how to do this but for another PR.

Basically what we would do is a "FlagsForFile" for python? I mean we should make a way for the user to specify how their project is made 😕

jedi.settings.additional_dynamic_modules

What I'm worried about with this is that there are some library which are really expensive to load and to work with and using only what is loaded (like jedi do right now) is better IMHO, but I could be wrong in this, not sure.

This is already affecting the GoToReferences subcommand so I don't see why we would ship GoToReferences but not RefactorRename (both commands are using the usages request).

This is absolutely true. Should we put it in the docs even if it is jedi behaviour?

[micbou]

Basically what we would do is a "FlagsForFile" for python? I mean we should make a way for the user to specify how their project is made 😕

Yes, I have a prototype in this branch. Users create a .ycm_extra_conf.py file at the root of their projects and implement the PythonSettings function (we should probably rename FlagsForFile to something like ClangFlagsForFile). This function takes **kwargs as its arguments (for the g:ycm_extra_conf_vim_data option) and must return a dictionary with the key additional_sources. Its value is passed to JediHTTP which then pass it to jedi.settings.additional_dynamic_modules. It's up to the user to set the sources.

Here's a .ycm_extra_conf.py example (found in my branch and used for ycmd):

import os
import fnmatch

DIR_OF_CURRENT_SCRIPT = os.path.dirname( os.path.abspath( __file__ ) )


def GetAdditionalSources():
  additional_sources = []
  for root, dirnames, filenames in os.walk( DIR_OF_CURRENT_SCRIPT ):
    for filename in fnmatch.filter( filenames, '*.py' ):
      if not filename.endswith( '.ycm_extra_conf.py' ):
        additional_sources.append( os.path.join( root, filename ) )
  return additional_sources


def PythonSettings( **kwargs ):
  return {
    'additional_sources': GetAdditionalSources()
  }

What do you think of this approach? We can easily extend it to preloaded imports.

What I'm worried about with this is that there are some library which are really expensive to load and to work with and using only what is loaded (like jedi do right now) is better IMHO, but I could be wrong in this, not sure.

This seems to work fine for a project like ycmd (with 118 files added!). Need more testing though.

This is absolutely true. Should we put it in the docs even if it is jedi behaviour?

I think so. We could add a note like "This only works for the current file and all files directly or indirectly imported by this file" for the GoToReferences and RefactorRename subcommands.

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[Valloric]

@micbou I'm fine with a PythonSettings function here, seems like we need it. Renaming FlagsForFile should be a separate discussion.

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[micbou]

Renaming FlagsForFile should be a separate discussion.

I agree. Just a note for later.

---

Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[puremourning]

We had a chat about this. In summary, here are my thoughts:

• I'm cool with the idea - I think it's good
• I think we should just call the method JediSettings and have it return a jedi.settings dictionary which we pass as-is to JediHTTP and them to jedi.
• If JediHTTP or ycmd require certain settings to be set, we just flat out override what the user supplies
• If we want to allow other settings (non-Jedi) then we could use something like PythonSettings returning something like:

def PythonSettings( **kwargs ):
  return {
    'jedi_settings': { ... jedi settings object ... },
    'ycmd_rocks': True,
  }

This makes the code simple and avoids lots of pernickety translations which are potentially somewhat brittle.

You ask about being a separate PR, but I feel like we need this ability for renames (which are destructive) to be reasonably shippable. Can be 2 PRs, but I think we need to merge both before we announce the feature ;)

Renaming FlagsForFile would just be candy - we'd have to keep the old name for the existing thousands of users ;)

[puremourning]

On reflection jedi.settings is a module not a dictionary, so this might turn out a little more convoluted. If it turns out to be hard or requiring too much cleverness, then we can just expose those settings we think are useful.

[Valloric]

I agree that we should namespace jedi options under a jedi-specific key. That leaves us room for the future.

[homu]

☔ The latest upstream changes (presumably #513) made this pull request unmergeable. Please resolve the merge conflicts.

[Valloric]

@micbou

Since it's been 2 years since this PR has been updated, I think it's safe to close it. 😉