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.

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

AutoImport imports if already imported #194

Open
erikw opened this issue Dec 9, 2016 · 10 comments
Open

AutoImport imports if already imported #194

erikw opened this issue Dec 9, 2016 · 10 comments

Comments

@erikw
Copy link

erikw commented Dec 9, 2016

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
Copy link
Contributor

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

Thanks for reporting!

@sergeyglazyrindev
Copy link
Contributor

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

@soupytwist soupytwist assigned soupytwist and unassigned soupytwist Dec 9, 2016
@soupytwist
Copy link
Contributor

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

@emacsway
Copy link
Contributor

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

@sergeyglazyrindev
Copy link
Contributor

sergeyglazyrindev commented Dec 22, 2016

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
Copy link
Contributor

mcepl commented Dec 23, 2016

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
Copy link
Contributor

sergeyglazyrindev commented Dec 23, 2016

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
Copy link
Contributor

mcepl commented Dec 24, 2016

Doesn’t ropemacs use ropemode as well?

@sergeyglazyrindev
Copy link
Contributor

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

@bagel897
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants