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
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e7e7dd2
Auto import actions using importmagic
youben11 Oct 28, 2019
f9ef0a0
import re
youben11 Oct 28, 2019
4c89036
typo
youben11 Oct 28, 2019
4383f1e
test against a specific action
youben11 Oct 28, 2019
7ed8a3b
disable actions test
youben11 Oct 29, 2019
4ae20c8
missing white-space
youben11 Oct 29, 2019
8242066
separate action generation logic: fix linting problem
youben11 Oct 29, 2019
5175028
fix: pass document instead of source
youben11 Oct 29, 2019
f54910d
filter results based on minScore parameter
youben11 Oct 29, 2019
2668de3
update test
youben11 Oct 29, 2019
ecabe85
Merge branch 'develop' into importmagic
gatesn Nov 4, 2019
936f121
private function
youben11 Nov 4, 2019
b0f04a3
change log level
youben11 Nov 4, 2019
2261042
Merge branch 'importmagic' of github.com:youben11/python-language-ser…
youben11 Nov 4, 2019
d57d904
warn about unreferenced import/variable/function
youben11 Nov 6, 2019
c7e5b36
remove unreferenced imports
youben11 Nov 9, 2019
061f0cc
config option to enable importmagic
youben11 Nov 12, 2019
e9996f7
fix camel_to_underscore
youben11 Nov 12, 2019
48d1f01
clarify remove command action title
youben11 Nov 12, 2019
2d12e54
clean message checking
youben11 Nov 12, 2019
71b8385
get diagnostics from pyls_lint
youben11 Nov 12, 2019
f99d33e
non blocking index build
youben11 Nov 12, 2019
6812993
store cache as dict
youben11 Nov 13, 2019
d178670
test remove action
youben11 Nov 14, 2019
ae63149
better linting test
youben11 Nov 14, 2019
a712a73
search symbols in source tokens instead of raw string:
youben11 Nov 15, 2019
80def3f
check for None value
youben11 Nov 15, 2019
0a9d163
fix range
youben11 Nov 15, 2019
e7ee0ca
use backward compatible tokenizer func
youben11 Nov 15, 2019
809a4af
fix indexing and pattern matching
youben11 Nov 15, 2019
1c509f9
typo
youben11 Nov 15, 2019
b82ff8f
new style class
youben11 Nov 15, 2019
107c431
use list comprehension for better perf
youben11 Nov 15, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions pyls/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import os
import sys
import threading
import re

PY2 = sys.version_info.major == 2

Expand Down Expand Up @@ -192,3 +193,7 @@ def is_process_alive(pid):
return e.errno == errno.EPERM
else:
return True


def camel_to_underscore(camelcase):
return re.sub('([A-Z]+)', r'_\1', camelcase).lower()
youben11 marked this conversation as resolved.
Show resolved Hide resolved
187 changes: 187 additions & 0 deletions pyls/plugins/importmagic_lint.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,187 @@
# Copyright 2017 Palantir Technologies, Inc.
import logging
import re
import sys
import importmagic
from pyls import hookimpl, lsp, _utils


log = logging.getLogger(__name__)

SOURCE = 'importmagic'
ADD_IMPORT_COMMAND = 'importmagic.addimport'
MAX_COMMANDS = 4
UNRES_RE = re.compile(r"Unresolved import '(?P<unresolved>[\w.]+)'")

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

"""Build index of symbols from python modules.
Cache the index so we don't build it multiple times unnecessarily.
"""
key = tuple(sys_path)
if key not in _index_cache:
log.debug("Started building importmagic index")
youben11 marked this conversation as resolved.
Show resolved Hide resolved
index = importmagic.SymbolIndex()
# The build tend to be noisy
logging.getLogger('importmagic.index').setLevel(logging.ERROR)
youben11 marked this conversation as resolved.
Show resolved Hide resolved
index.build_index(paths=sys_path)
_index_cache[key] = index
logging.getLogger('importmagic.index').setLevel(logging.DEBUG)
log.debug("Finished building importmagic index")
return _index_cache[key]


@hookimpl
def pyls_commands():
return [ADD_IMPORT_COMMAND]


@hookimpl
def pyls_lint(document):
"""Build a diagnostics of unresolved symbols. Every entry follows this format:
{
'source': 'importmagic',
'range': {
'start': {
'line': start_line,
'character': start_column,
},
'end': {
'line': end_line,
'character': end_column,
},
},
'message': 'Unresolved import <symbol>',
'severity': lsp.DiagnosticSeverity.Hint,
}

Args:
document: The document to be linted.
Returns:
A list of dictionaries.
"""
scope = importmagic.Scope.from_source(document.source)
unresolved, _unreferenced = scope.find_unresolved_and_unreferenced_symbols()
youben11 marked this conversation as resolved.
Show resolved Hide resolved

diagnostics = []

# Annoyingly, we only get the text of an unresolved import, so we'll look for it ourselves
for unres in unresolved:
if unres not in document.source:
continue

for line_no, line in enumerate(document.lines):
pos = line.find(unres)
if pos < 0:
continue

diagnostics.append({
'source': SOURCE,
'range': {
'start': {'line': line_no, 'character': pos},
'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.

})

return diagnostics


@hookimpl
def pyls_code_actions(config, document, context):
"""Build a list of actions to be suggested to the user. Each action follow this format:
{
'title': 'importmagic',
'command': command ('importmagic.add_import'),
'arguments':
{
'uri': document.uri,
'version': document.version,
'startLine': start_line,
'endLine': end_line,
'newText': text,
}
}
"""
# Update the style configuration
conf = config.plugin_settings('importmagic_lint')
min_score = conf.get('minScore', 1)
log.debug("Got importmagic settings: %s", conf)
importmagic.Imports.set_style(**{_utils.camel_to_underscore(k): v for k, v in conf.items()})

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.

if diagnostic.get('source') != SOURCE:
continue
m = UNRES_RE.match(diagnostic['message'])
if not m:
continue

unres = m.group('unresolved')
# Might be slow but is cached once built
index = _get_index(sys.path)

for score, module, variable in sorted(index.symbol_scores(unres)[:MAX_COMMANDS], reverse=True):
if score < min_score:
# Skip low score results
continue

actions.append(generate_add_action(document, index, module, variable))

return actions


def generate_add_action(document, index, module, variable):
youben11 marked this conversation as resolved.
Show resolved Hide resolved
# Generate the patch we would need to apply
imports = importmagic.Imports(index, document.source)
if variable:
imports.add_import_from(module, variable)
else:
imports.add_import(module)
start_line, end_line, text = imports.get_update()

action = {
'title': _command_title(variable, module),
'command': ADD_IMPORT_COMMAND,
'arguments': [{
'uri': document.uri,
'version': document.version,
'startLine': start_line,
'endLine': end_line,
'newText': text
}]
}
return action


@hookimpl
def pyls_execute_command(workspace, command, arguments):
if command != ADD_IMPORT_COMMAND:
return

args = arguments[0]

edit = {'documentChanges': [{
'textDocument': {
'uri': args['uri'],
'version': args['version']
},
'edits': [{
'range': {
'start': {'line': args['startLine'], 'character': 0},
'end': {'line': args['endLine'], 'character': 0},
},
'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



def _command_title(variable, module):
if not variable:
return 'Import "%s"' % module
return 'Import "%s" from "%s"' % (variable, module)
3 changes: 3 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
'all': [
'autopep8',
'flake8',
'importmagic',
'mccabe',
'pycodestyle',
'pydocstyle>=2.0.0',
Expand All @@ -58,6 +59,7 @@
],
'autopep8': ['autopep8'],
'flake8': ['flake8'],
'importmagic': ['importmagic'],
'mccabe': ['mccabe'],
'pycodestyle': ['pycodestyle'],
'pydocstyle': ['pydocstyle>=2.0.0'],
Expand All @@ -78,6 +80,7 @@
'pyls': [
'autopep8 = pyls.plugins.autopep8_format',
'flake8 = pyls.plugins.flake8_lint',
'importmagic = pyls.plugins.importmagic_lint',
'jedi_completion = pyls.plugins.jedi_completion',
'jedi_definition = pyls.plugins.definition',
'jedi_hover = pyls.plugins.hover',
Expand Down
68 changes: 68 additions & 0 deletions test/plugins/test_importmagic_lint.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
# Copyright 2019 Palantir Technologies, Inc.
import tempfile
import os
from pyls import lsp, uris
from pyls.plugins import importmagic_lint
from pyls.workspace import Document

DOC_URI = uris.from_fs_path(__file__)
DOC = """
time.sleep(10)
print("test")
"""


def temp_document(doc_text):
temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False)
name = temp_file.name
temp_file.write(doc_text)
temp_file.close()
doc = Document(uris.from_fs_path(name))

return name, doc


def test_importmagic_lint():
try:
name, doc = temp_document(DOC)
diags = importmagic_lint.pyls_lint(doc)
unres_symbol = [d for d in diags if d['source'] == 'importmagic'][0]

assert unres_symbol['message'] == "Unresolved import 'time.sleep'"
assert unres_symbol['range']['start'] == {'line': 1, 'character': 0}
assert unres_symbol['range']['end'] == {'line': 1, 'character': 10}
assert unres_symbol['severity'] == lsp.DiagnosticSeverity.Hint

finally:
os.remove(name)


def test_importmagic_actions(config):
context = {
'diagnostics': [
{
'range':
{
'start': {'line': 1, 'character': 0},
'end': {'line': 1, 'character': 10}
},
'message': "Unresolved import 'time.sleep'",
'severity': lsp.DiagnosticSeverity.Hint,
'source': importmagic_lint.SOURCE
}
]
}

try:
name, doc = temp_document(DOC)
actions = importmagic_lint.pyls_code_actions(config, doc, context)
action = [a for a in actions if a['title'] == 'Import "time"'][0]
arguments = action['arguments'][0]

assert action['command'] == importmagic_lint.ADD_IMPORT_COMMAND
assert arguments['startLine'] == 1
assert arguments['endLine'] == 1
assert arguments['newText'] == 'import time\n\n\n'

finally:
os.remove(name)
6 changes: 6 additions & 0 deletions test/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -87,3 +87,9 @@ def test_clip_column():
assert _utils.clip_column(2, ['123\n', '123'], 0) == 2
assert _utils.clip_column(3, ['123\n', '123'], 0) == 3
assert _utils.clip_column(4, ['123\n', '123'], 1) == 3


def test_camel_to_underscore():
assert _utils.camel_to_underscore('camelCase') == 'camel_case'
assert _utils.camel_to_underscore('hangClosing') == 'hang_closing'
assert _utils.camel_to_underscore('ignore') == 'ignore'
youben11 marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions vscode-client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,11 @@
},
"uniqueItems": true
},
"pyls.plugins.importmagic_lint.minScore": {
youben11 marked this conversation as resolved.
Show resolved Hide resolved
"type": "number",
"default": 1,
"description": "The minimum score used to filter module import suggestions."
},
"pyls.plugins.jedi_completion.enabled": {
"type": "boolean",
"default": true,
Expand Down