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

Code action to add/delete imports #685

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from

Conversation

youben11
Copy link
Contributor

@youben11 youben11 commented Oct 28, 2019

This is based on the previous work of @gatesn

  • Actions to import suggested modules based on the index of symbols of all available python modules (might be slow to build but cached for future use)
  • Linting hint about unresolved symbols only as we aren't sure if those symbols aren't just variables.

Last features

  • Actions to remove unreferenced imports
  • Linting warning about unreferenced imports, variables and functions

related #86

@youben11
Copy link
Contributor Author

@gatesn is there any way to make the call to pyls_code_actions non blocking?

pyls/plugins/importmagic_lint.py Outdated Show resolved Hide resolved
pyls/plugins/importmagic_lint.py Outdated Show resolved Hide resolved
pyls/plugins/importmagic_lint.py Outdated Show resolved Hide resolved
pyls/plugins/importmagic_lint.py Outdated Show resolved Hide resolved
@youben11 youben11 changed the title Code action to add import Code action to add/delete imports Nov 4, 2019
@zewa666
Copy link

zewa666 commented Nov 5, 2019

@youben11 I've checked out your version and tried it with a Webdriver/Selenium sample. Works fantastic to e.g autoimport from selenium import webdriver. What doesn't work though is importing local references from another file. Is that the supposed way how importmagic works?

@youben11
Copy link
Contributor Author

youben11 commented Nov 9, 2019

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/_utils.py Outdated Show resolved Hide resolved
_index_cache = {}


def _get_index(sys_path):
Copy link
Contributor

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

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

Copy link
Contributor Author

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.

pyls/plugins/importmagic_lint.py Outdated Show resolved Hide resolved
index = _get_index(sys.path)
actions = []
diagnostics = context.get('diagnostics', [])
for diagnostic in diagnostics:
Copy link
Contributor

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', [])]

Copy link
Contributor Author

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

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

pyls/plugins/importmagic_lint.py Outdated Show resolved Hide resolved
test/test_utils.py Show resolved Hide resolved
vscode-client/package.json Show resolved Hide resolved
@youben11 youben11 force-pushed the importmagic branch 3 times, most recently from 4c501a4 to 6812993 Compare November 14, 2019 08:43
@youben11
Copy link
Contributor Author

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).
@gatesn
Copy link
Contributor

gatesn commented Nov 15, 2019

@youben11 perhaps you could use Jedi's names() call instead of walking the AST yourself. It may help, haven't verified: https://jedi.readthedocs.io/en/latest/docs/api.html#jedi.names

@youben11
Copy link
Contributor Author

youben11 commented Nov 15, 2019

@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.

Update

Actually there is a backward compatible function to use that works for both version.

@gjabell
Copy link

gjabell commented Jul 7, 2020

Any motion on this PR? Would love to have this functionality in the language server!

@weeman1337
Copy link

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

@ccordoba12
Copy link
Contributor

ccordoba12 commented May 2, 2021

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.

@Mte90
Copy link

Mte90 commented Mar 5, 2024

If someone is looking for the same feature in the latest pylsp: python-lsp/python-lsp-server#199

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

Successfully merging this pull request may close these issues.

None yet

7 participants