Repository navigation
importmagic implemented - #636
Vladimir Urushev (pilat) wants to merge 3 commits into
Conversation
|
|
Codecov Report
@@ Coverage Diff @@
## master #636 +/- ##
=========================================
+ Coverage 60.69% 61.09% +0.4%
=========================================
Files 232 236 +4
Lines 10502 10825 +323
Branches 1830 1889 +59
=========================================
+ Hits 6374 6614 +240
- Misses 4122 4205 +83
Partials 6 6
Continue to review full report at Codecov.
|
|
Thanks for the PR, Vladimir Urushev (@pilat) ! We will have a look at this when we have the time (should be some time in February as we just hit code freeze for our next release). |
| public async provideCompletionItems(document: vscode.TextDocument, position: vscode.Position, | ||
| token: vscode.CancellationToken, context: CompletionContext): Promise<vscode.CompletionItem[]> { | ||
| if (!this.enable) { | ||
| return Promise.resolve([]); |
There was a problem hiding this comment.
Question: why not return [] instead of Promise.resolve([]) here and in other async functions, as async functions return promises automatically? That matches the current style of project.
There was a problem hiding this comment.
Oh, you're right! I have already fixed it.
|
Vladimir Urushev (@pilat) |
|
Don Jayamanne (@DonJayamanne) |
| json_message = {'error': True, 'message': 'Importmagic not installed', | ||
| 'traceback': '', 'type': 'ModuleNotFoundError'} | ||
| sys.stderr.write(json.dumps(json_message)) | ||
| sys.stderr.write('\n') |
There was a problem hiding this comment.
How about:
from __future__ import print_function
print(json.dumps(json_message), file=sys.stderr)| method_name = request.get('method') | ||
|
|
||
| result = None | ||
| method = getattr(self, 'method_%s' % method_name) |
There was a problem hiding this comment.
It would be much more readable with a simple if/elif/elif switch. Dynamically constructing method names and using getattr to access code means that you will never be able to find where a particular function is used.
| self._input = io.open(sys.stdin.fileno(), encoding='utf-8') | ||
| self._index = None | ||
|
|
||
| def method_build_index(self, **kwargs): |
There was a problem hiding this comment.
Why are these named method_*?
| python_source = fd.read() | ||
| scope = importmagic.Scope.from_source(python_source) | ||
|
|
||
| _unresolved, _unreferenced = \ |
There was a problem hiding this comment.
Leading underscore typically denotes unused variables declared only for unpacking:
foo, _dummy = function_that_returns_a_tuple()
| raise ValueError('Empty module') | ||
|
|
||
| if not self._index: | ||
| raise Exception('First run inibuild_indext() method') |
There was a problem hiding this comment.
There is no inibuild_indext.
| python_source = fd.read() | ||
|
|
||
| imports = importmagic.Imports(self._index, python_source) | ||
| imports.set_style(multiline='backslash', max_columns=79) # TODO: Get style from VS Code |
There was a problem hiding this comment.
This will likely become a blocker for merging this change. You could import isort and use its configuration parser to determine proper settings for the file. This would make sure whatever this tool does is not immediately flagged by an isort checker.
| result = method(**request) | ||
|
|
||
| if not result or type(result) != dict: | ||
| raise ValueError('Method must return dict') |
There was a problem hiding this comment.
This looks like something that should be covered by tests rather than a runtime check.
| response = self._process_request(request) | ||
| json_message = dict(id=request_id, **response) | ||
| self._success_response(json_message) | ||
| except: |
There was a problem hiding this comment.
Unqualified except is bad. It will capture unexpected things, like SystemExit or KeyboardInterrupt.
|
Gentle ping, Vladimir Urushev (@pilat). It's a really nice feature I'd like to see merged. |
|
There's currently no plans on accepting this PR as it has merge conflicts which have to get fixed first. We are also working on some intellisense changes which should help with getting this feature solved as well at some point, even if we don't get this specific PR merged. |
|
This is a really nice feature that would save many Python devs a ton of time. It's been around a while: DonJayamanne/pythonVSCode#764. I hope we'll see this in production soon! Vladimir Urushev (@pilat): is there a way to test this feature out on a local version? |
|
Just wondering, what's the status of this issue? |
|
Timur (@timonweb) we have an experimental analysis engine that is being developed which will enable this, so we're in a weird spot where we could either merge this now and then probably toss the work later, or simply wait for the new engine to land and make that do this. |
|
Mikhail Arkhipov (@MikhailArkhipov) do you think we could re-use this approach in the Python language server? When would be the right time to start looking at it? |
|
Brett Cannon (@brettcannon), is there an update on this or the experimental analysis engine? |
|
thatneat it's basically at the top of our roadmap to add this to the language server (after fixing bugs of course 😉 ). |
|
Looks like the thing to do is use this extension instead: https://marketplace.visualstudio.com/items?itemName=brainfit.vscode-importmagic |
|
I'm going to close this as the plan is to put this functionality into the language server. Thanks to Vladimir Urushev (@pilat) for taking the time to create this PR! |
|
awesome, can't wait! Is there somewhere we can track the corresponding feature progress on the language server? Or should we just watch changelogs to find out when this comes out? |
13 participants
I tried to implement python importmagic. Perhaps it will be resolved issue #21
It works like this