Skip to content

PR: Fix Inline comments including text with import cause functions/classes to disappear when under cells in the Outline explorer #639

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

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

Conversation

jsbautista
Copy link

Fix Inline comments including text with import cause functions/classes to disappear when under cells in the Outline explorer

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening the PR @jsbautista!

The proposed change makes sense, but I think it misses a few cases:

  • from . import something
  • from ..module import something
  • from module import (a, b)

It will also fail with multi-line imports, but after all the existing code is not handling multi-line imports well either.

Would you mind adding a test for the various cases in:

def test_symbols(config, workspace):
doc = Document(DOC_URI, workspace, DOC)
config.update({"plugins": {"jedi_symbols": {"all_scopes": False}}})
symbols = pylsp_document_symbols(config, doc)
# All four symbols (import sys, a, B, main)
# y is not in the root scope, it shouldn't be returned
assert len(symbols) == 5
def sym(name):
return [s for s in symbols if s["name"] == name][0]
# Check we have some sane mappings to VSCode constants
assert sym("a")["kind"] == SymbolKind.Variable
assert sym("B")["kind"] == SymbolKind.Class
assert sym("main")["kind"] == SymbolKind.Function
# Not going to get too in-depth here else we're just testing Jedi
assert sym("a")["location"]["range"]["start"] == {"line": 2, "character": 0}
# Ensure that the symbol range spans the whole definition
assert sym("main")["location"]["range"]["start"] == {"line": 9, "character": 0}
assert sym("main")["location"]["range"]["end"] == {"line": 12, "character": 0}
def test_symbols_all_scopes(config, workspace) -> None:
doc = Document(DOC_URI, workspace, DOC)
symbols = pylsp_document_symbols(config, doc)
helper_check_symbols_all_scope(symbols)

An alternative to using a regular expression would be using the ast module which should properly parse import expressions and ignore comments.

Comment on lines 31 to 34
pattern = (
re.compile
(r'^\s*(?!#)(from\s+\w+(\.\w+)*\s+import\s+[\w,\s*]+|import\s+[\w,\s]+)')
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would like to take advantage of compiling it once, you should move the pattern definition up, prior to the function definition, otherwise it will just compile it every time.

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.

2 participants