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.

Sign up for GitHub

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

Add ignorePatterns option to filter completions with name regex #169

Merged
merged 2 commits into from
Oct 25, 2021
Merged

Add ignorePatterns option to filter completions with name regex #169

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
231f4b1
Add ignorePatterns option to filter completions with name regex
pappasam Oct 19, 2021
565d109
Add comment explaining performance optimization
pappasam Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,8 @@ If you are configuring manually, jedi-language-server supports the following [in
},
"completion": {
"disableSnippets": false,
"resolveEagerly": false
"resolveEagerly": false,
"ignorePatterns": []
},
"diagnostics": {
"enable": true,
Expand Down
3 changes: 2 additions & 1 deletion jedi_language_server/initialization_options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
initialization options.
"""

from typing import List, Optional, Set
from typing import List, Optional, Pattern, Set

from pydantic import BaseModel, Field
from pygls.lsp.types import MarkupKind
Expand Down Expand Up @@ -34,6 +34,7 @@ class CodeAction(Model):
class Completion(Model):
disable_snippets: bool = False
resolve_eagerly: bool = False
ignore_patterns: List[Pattern] = []


class Diagnostics(Model):
Expand Down
14 changes: 11 additions & 3 deletions jedi_language_server/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,23 @@ def completion(
server: JediLanguageServer, params: CompletionParams
) -> Optional[CompletionList]:
"""Returns completion items."""
snippet_disable = server.initialization_options.completion.disable_snippets
resolve_eagerly = server.initialization_options.completion.resolve_eagerly
ignore_patterns = server.initialization_options.completion.ignore_patterns
document = server.workspace.get_document(params.text_document.uri)
jedi_script = jedi_utils.script(server.project, document)
jedi_lines = jedi_utils.line_column(params.position)
completions_jedi = jedi_script.complete(*jedi_lines)
if not ignore_patterns:
completions_jedi = (comp for comp in jedi_script.complete(*jedi_lines))
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this needs to be a special case, does it?

Copy link
Owner Author

Choose a reason for hiding this comment

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

I'm not strongly opposed to removing it, but I included this special case as an optimization. Maybe the readability trade-off isn't worth it, so I'm curious to hear your thoughts!

My thought is that most users won't configure ignore_patterns. Since some calls to jedi_script.complete may return many completions, I believe special casing the empty ignore_patterns allows me to avoid repeated calls to a filtering function in the generator comprehension on line 199 for most users (or at least for me).

Copy link
Contributor

Choose a reason for hiding this comment

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

Well I also don't feel very strongly about this!

My instinct would be to avoid special cases unless they are needed, you're suggesting that perhaps the special case is needed, for performance reasons.

I guess if we really wanted to decide which is 'right' then we should do some profiling to figure out whether the filtering is actually meaningfully expensive - but honestly that sounds like much more trouble than just making a decision.

Maybe a one-line comment explaining the motivation? I think I had suspected that you believed that these cases needed to be different rather than that this was a deliberate optimisation. But this discussion right here is probably already more than this whole thing deserves!

Copy link
Owner Author

Choose a reason for hiding this comment

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

👍

else:
completions_jedi = (
comp
for comp in jedi_script.complete(*jedi_lines)
if not any(i.match(comp.name) for i in ignore_patterns)
)
snippet_support = server.client_capabilities.get_capability(
"text_document.completion.completion_item.snippet_support", False
)
snippet_disable = server.initialization_options.completion.disable_snippets
resolve_eagerly = server.initialization_options.completion.resolve_eagerly
markup_kind = _choose_markup(server)
is_import_context = jedi_utils.is_import(
script_=jedi_script,
Expand Down