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

implement AddImport - #1656

Merged
Heejae Chang (heejaechang) merged 47 commits into
microsoft:masterfrom
heejaechang:addimport
Oct 29, 2019
Merged

Heejae Chang (heejaechang) merged 47 commits into
microsoft:masterfrom
heejaechang:addimport

Conversation

@heejaechang

@heejaechang Heejae Chang (heejaechang) commented Oct 9, 2019 •

Copy link
Copy Markdown

Closes #19.

provides add import code action for unknown symbols.

...

  • basic implementation for importing "modules" including submodules + members from loaded modules
  • add tests
  • add add import option [decide not to add option for now]
  • add support for members from unloaded modules
  • improve add import code action ordering a bit
  • open related issue on a relative path (provide refactoring to convert full module name to relative module name in import #1707)
  • provide abbreviation for well-known modules.
  • handle context. ex) specific import based on context (only import type/function and etc)
  • perf ex) cache lint result. run lint only for specific diagnostics and etc - now uses diagnostic service cache
  • show quick fix in hover
  • support reversed abbreviation for well-known modules
  • support unique name on abbreviation

addimport2

currently it does only basic behavior and works only for module known to path resolver and loaded modules (variable module)

no test yet. require many refactorings
the reason it was assert was due to code like below that defines 2 functions with same signature and we only keep 1 symbol out of it.
...
       def f(x):
            class C:
                x = 12
                def m(self):
                    return x
                locals()
            return C

        self.assertEqual(f(1).x, 12)

        def f(x):
            class C:
                y = x
                def m(self):
                    return x
                z = list(locals())
            return C
...
added just one since it is very expensive.
@heejaechang Heejae Chang (heejaechang) changed the title [WIP] implement AddImport implement AddImport Oct 14, 2019
…lready loaded by users code.

for closed files that are never loaded in the editor before, it will use SymbolIndexer to find candidates.

but since the indexer is syntax based, it wont be able to provide information that require semantic info such as imported members

ex) from os import * or __all__.extend(core.__all__)

which means we will not propose

from os.path import join

if os module is not already loaded, instead we will propose ntpath or mac/posixpath since that is where join is actually defined.

unfortunately we can't load modules just for add import, it is too expensive.

we might in future add another index that is based on analyzed data which could have correct info saved. but for now, this is what we decided.
@heejaechang

Copy link
Copy Markdown
Author

made not to load modules. can you take another look?

now we show them at the top if there is any.
@heejaechang

Copy link
Copy Markdown
Author

Mikhail Arkhipov (@MikhailArkhipov) any more feedback?

@Astrantia

Copy link
Copy Markdown

Heejae Chang (@heejaechang) Will it be possible to add an auto import feature? Importing with a quickfix is great, but it's the best when unresolved imports are auto imported on save.

@jakebailey

Jake Bailey (jakebailey) commented Oct 28, 2019 •

Copy link
Copy Markdown
Member

There are so many options for what a particular unresolved name could be that it might be impractical to do it right on save with no user input, or in cases where the unresolved name is just a local error (and not intended to be an import). We're going to start simple and go from there.

(Also note that on-save may be more of a formatting operation, which we don't currently have plans to handle directly.)

@MikhailArkhipov

Copy link
Copy Markdown

Heejae Chang (@heejaechang) - I'd test it with Anaconda to be sure there are no performance issues and demo it to Tyreke White (@Anapo14) to make sure behavior matches spec expectations. Otherwise LGTM.

@heejaechang

Copy link
Copy Markdown
Author

thank you!

@heejaechang
Heejae Chang (heejaechang) merged commit 254c8f1 into microsoft:master Oct 29, 2019
@grisaitis

William C Grisaitis (grisaitis) commented Jan 13, 2020 •

Copy link
Copy Markdown

i see that 254c8f1 is not included in any releases, just master. does this mean it's not available in stable vs code?

edit: sorry if this is the wrong place to ask this. i'm pretty confused by the various repos (microsoft/vscode-python, microsoft/python-language-server, etc)

@jakebailey

Copy link
Copy Markdown
Member

See #19 (comment) (where you also asked this). You can ignore the GitHub releases.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement code action for missing imports

6 participants