Skip to content

Don't go to definition of builtin modules #687

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

Merged
merged 6 commits into from
Nov 1, 2019

Conversation

jackwickham
Copy link
Contributor

@jackwickham jackwickham commented Oct 31, 2019

If you do Go To Definition (F12 in vscode) on a string, it opens the builtins.pyi file for python 3, because the definition is not being filtered properly.

This PR corrects that behaviour so that neither python 2 nor 3 will go to the builtins file.

This doesn't affect the behaviour with imported builtins - they still respect the configuration.

@jackwickham
Copy link
Contributor Author

If going to builtin definitions would be desirable for some people then it could be made configurable, but I'm not sure if that's necessary.

@@ -40,7 +40,7 @@ def test_builtin_definition(config):

# No go-to def for builtins
doc = Document(DOC_URI, DOC)
assert len(pyls_definitions(config, doc, cursor_pos)) == 1
assert [] == pyls_definitions(config, doc, cursor_pos)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be assert not pyls_definitions(config, doc, cursor_pos)

@@ -21,5 +21,6 @@ def pyls_definitions(config, document, position):
}
}
for d in definitions
if d.is_definition() and d.line is not None and d.column is not None and d.module_path is not None
if d.is_definition() and d.line is not None and d.column is not None and
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you pull this out into a function? It doesn't read very clearly anymore

]


def not_internal_definition(definition):
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefix with _ :)

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.

3 participants