python-rope / rope

a python refactoring library
GNU Lesser General Public License v3.0
1.91k stars 160 forks source link

AutoImport imports if already imported #194

Open erikw opened 7 years ago

erikw commented 7 years ago

AutoImport should not import the given name if it is already imported. Issuing this command many times on the same symbol will just append more imports

from X import Y
from X import Y
from X import Y
from X import Y
soupytwist commented 7 years ago

I'll take a look at this soon, it's definitely a bug.

Thanks for reporting!

sergeyglazyrindev commented 7 years ago

Hey Nick. I'll check it on next week. Please assign this issue to me.

soupytwist commented 7 years ago

Github won't let me do that, lame. It's all yours!

emacsway commented 7 years ago

@soupytwist , probably, @sergeyglazyrindev should be added as member first.

sergeyglazyrindev commented 7 years ago

Hey guys! I am not sure this is an issue with rope Autoimport stuff. It seems like an issue in ropemode. https://github.com/python-rope/ropemode/blob/master/ropemode/interface.py#L606-L613 Ropevim uses ropemode to communicate with rope and editor. All what does ropemode is simply asking a list of globally registered modules https://github.com/python-rope/rope/blob/master/rope/contrib/autoimport.py#L56-L62 and then if such name appears in current module and if there's only one such name, it automatically inserts import It seems to me it's not an issue but it works by design. According to what I understand in ropemode it works following way: https://github.com/python-rope/ropemode/blob/master/ropemode/interface.py#L605 it asks vim for currently cursored word and looking for such global module and if it exists inserts import though if there are two global modules with the same name, it asks which module would you like to insert. That's why I think it works as designed. But it raises few more questions: what if we decide we want to add here workaround, we need to add it to ropemode because:

  1. in theory it should be a job connected to the code where we have an access to currently edited text (edited text could be not saved, so we can't rely on observing changes in a file, that's why it can't be handled in rope), ropemode provides us such access, we may add such workaround to ropemode: https://github.com/python-rope/ropemode/blob/master/ropemode/interface.py#L659 if (this.env.current_file.has_text('from X import Y')): return
mcepl commented 7 years ago

if (this.env.current_file.has_text('from X import Y')): return

Great analysis! Thank you. Just one thought: don't we have some better way how to find which imports are present than simple text search?

sergeyglazyrindev commented 7 years ago

Yes, I thought about much deeper analysis. But I need to test it with ropevim and vim. Actually, I use emacs, so that's what I could to get just investigating the code sources. But as far as I understand, anyway, much proper solution should be related to analyzing text in code editor because the file could be changed and not saved, so, if we decide to implement it only in rope (which is not by design of autoimport module, as far as I understand) we will need anyway fallback to checking if such import statement present in code editor's opened file (to make it 100% working). So, I can tell a bit more about this problem in a week or so. Right now finalizing one important project.

mcepl commented 7 years ago

Doesn’t ropemacs use ropemode as well?

sergeyglazyrindev commented 7 years ago

Yes, it does. I'll try to do much deeper analysis a bit later.

bagel897 commented 2 years ago

In the sqllite implementation under #469 there is an option to pass a list of ignored names. I used this in https://github.com/python-lsp/python-lsp-server/pull/199 to ignore names already defined in the file.