-
Notifications
You must be signed in to change notification settings - Fork 285
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
Code action to add/delete imports #685
base: develop
Are you sure you want to change the base?
Conversation
@gatesn is there any way to make the call to pyls_code_actions non blocking? |
@youben11 I've checked out your version and tried it with a Webdriver/Selenium sample. Works fantastic to e.g autoimport |
Hi @zewa666, thank you for testing this functionality and providing feedback. Yeah the index of symbols is built from the python modules found under sys.path, but I will certainly add the local workspace so you can have suggestions to import from project files. |
pyls/plugins/importmagic_lint.py
Outdated
_index_cache = {} | ||
|
||
|
||
def _get_index(sys_path): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this return a future, that way we can skip results instead of blocking on indexing.
'end': {'line': line_no, 'character': pos + len(unres)} | ||
}, | ||
'message': "Unresolved import '%s'" % unres, | ||
'severity': lsp.DiagnosticSeverity.Hint, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this not be a warning / error? Or does it overlap with e.g. pyflakes results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a hint because we actually don't know if this is a module to be imported or just a non defined variable, so better not to trigger a false positive. The unreferenced symbols are flagged with Warning because we know in advance if the symbol is either an import or a varialbe/function and the error should be consistent.
index = _get_index(sys.path) | ||
actions = [] | ||
diagnostics = context.get('diagnostics', []) | ||
for diagnostic in diagnostics: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it's easier to just call pyls_lint(document)
, then try to find the context diagnostic in the results. That way we don't have to parse the messages, just have to do an equality check.
e.g. action_diags = [diag for diag in pyls_lint(document) if diag in context.get('diagnostics', [])]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True that the diagnostics list can grow bigger than the local diagnostics and we will just pass most of them. Also we don't need to do the equality check if we call pyls_lint() as all the diagnostics will be processed.
'newText': args['newText'] | ||
}] | ||
}]} | ||
workspace.apply_edit(edit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think newer versions of clients support putting the edit in the code action:
/**
* A code action represents a change that can be performed in code, e.g. to fix a problem or
* to refactor code.
*
* A CodeAction must set either `edit` and/or a `command`. If both are supplied, the `edit` is applied first, then the `command` is executed.
*/
export interface CodeAction {
/**
* A short, human-readable, title for this code action.
*/
title: string;
/**
* The kind of the code action.
*
* Used to filter code actions.
*/
kind?: CodeActionKind;
/**
* The diagnostics that this code action resolves.
*/
diagnostics?: Diagnostic[];
/**
* The workspace edit this code action performs.
*/
edit?: WorkspaceEdit;
/**
* A command this code action executes. If a code action
* provides an edit and a command, first the edit is
* executed and then the command.
*/
command?: Command;
}
But I'm not sure which client capability we should check. What you've got now is probably safer
ba5c166
to
e9996f7
Compare
95bffde
to
f99d33e
Compare
4c501a4
to
6812993
Compare
7be97a3
to
d178670
Compare
The importmagic doesn't return the location of unreferenced/unresolved symbols so we needed to look them up ourselves, the search logic was a simple naive .find() call which might return false positives (e.g 'os' is found in 'pos'). Next I will be looking on how to simply tokenize the source code and search among the tokens so we make sure the matching is correct. |
raw string matching lead to false positives (e.g 'os' if found under 'pos' but it's not the actual symbol that we were searching).
beec6a1
to
0a9d163
Compare
@youben11 perhaps you could use Jedi's |
@gatesn the disadvantage with jedi.names is that it does only return definitions, symbols such as function calls aren't returned. The implementation using tokenize is pretty straightforward, however, I have to deal with the two different version of python here as the function takes different parameters for py2 and py3. UpdateActually there is a backward compatible function to use that works for both version. |
226dc59
to
e7ee0ca
Compare
494e129
to
107c431
Compare
Any motion on this PR? Would love to have this functionality in the language server! |
Hi there! Thanks for working on the import function 👍 After reading the comments it is not clear to me what is left to do. Since I really want this feature I would be happy to contribute - if I would know hat to do :D |
This project is not maintained anymore. However, the community created a fork of it at https://github.com/python-lsp/python-lsp-server/ So please submit this PR there if you want to see it finally merged. |
If someone is looking for the same feature in the latest pylsp: python-lsp/python-lsp-server#199 |
This is based on the previous work of @gatesn
Last features
related #86