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

Don't offer completions / auto-imports for class/function/parameter/import alias names #163

Closed
anatoly-kor opened this issue Jul 23, 2020 · 12 comments
Assignees
Labels
enhancement New feature or request fixed in next version (main) A fix has been implemented and will appear in an upcoming version

Comments

@anatoly-kor
Copy link

Describe the bug
For tests automatically adds import for fixture. Pytest know where find out fixture(conftest.py), not required make auto import for it.

To Reproduce

  1. install pytest
  2. create conftest.py with that example code:
import pytest

@pytest.fixture()
def ExampleFixture():
    return ['This', 'is', 'example', 'fixture']
  1. create test_example.py with that code:
import pytest

def test_is_instance_list(ExampleFixture):
    assert is instance(ExampleFixture, list)

Expected behavior
Import for ExampleFixture is not added

VS Code extension or command-line
VS Code extension
Version: v2020.7.3

@judej judej added the waiting for user response Requires more information from user label Jul 23, 2020
@savannahostrowski
Copy link
Contributor

Hey @anatoly-kor!

Just to be clear, what you're describing is that you are seeing a completion that auto-imports the fixture and you wouldn't expect this behaviour?

@anatoly-kor
Copy link
Author

Hi @savannahostrowski. Yes! I mean when test(with fixture) is wrote, auto-import add imports, but doesn't must because for pytest not required import fixture modules.
Quote from docs:

You don’t need to import the fixture you want to use in a test, it automatically gets discovered by pytest.

@jakebailey
Copy link
Member

Thanks for confirming. In this case, the problem is that completions are offered in some contexts where they don't make sense (parameter, function, and class names). If you're defining a function, we shouldn't try to autocomplete a parameter name (because we would never know what the right names are for a brand new function).

@jakebailey jakebailey added enhancement New feature or request needs investigation Could be an issue - needs investigation and removed waiting for user response Requires more information from user labels Jul 24, 2020
@albireox
Copy link

albireox commented Aug 2, 2020

Seeing the same issue here. Maybe a context could be added to the auto-import feature to disable it when a conftest.py file is in the same directory.

Overall I must say I've found the auto-import feature to be more of a hurdle than anything else. When an auto-import gets added is usually a duplicate or something I didn't really want, and in most case the module from which I'm adding a class or function is not discovered. It would be nice if the feature could be disable completely.

@jakebailey
Copy link
Member

If you have feedback about what could be improved with auto-imports in general, we'd appreciate those in a separate issue. If we are offering something that's a duplicate or something you don't want, then that's something to solve. This specific issue is about offering completions for names we can't actually know (brand new funcs/classes/parameters) and would be hit regardless of the auto-import setting.

It would be nice if the feature could be disable completely.

That is #64, which we plan to do.

@jakebailey jakebailey changed the title Auto-import for pytest Don't offer completions / auto-imports for class/function/parameter names Aug 3, 2020
@albireox
Copy link

albireox commented Aug 3, 2020

Right, I didn't want to confuse the issue and #64 sounds good.

To avoid auto-importing fixtures, something that could be checked is if the object has the attribute _pytestfixturefunction and if so not offer auto-import.

@ivarstange
Copy link

I like the solution proposed in #178. Today, the setting python.analysis.autoImportCompletions allows either true or false, but instead it could be always, ask or never. That way we don't need special rules for specific test libraries such as pytest.

@jakebailey
Copy link
Member

jakebailey commented Aug 10, 2020

That level of control over completions is not possible in VS Code as it stands now. We return a list of completions given a location in the code, and VS Code displays it. We can't defer this or ask for user input if they want to expand some extra stuff, because this is effectively a request for state where there's only one answer given the current document and location.

@WayneLambert
Copy link

The auto import feature is really useful in most scenarios, however as stated in this issue, it can be a little inconvenient with pytest when you're entering a fixture into your test function's signature and it auto-imports. Pytest does not require you to import its fixtures from a conftest.py file since its test discovery process looks for that filename.

By the design of the pytest library, users use a filename that is always called conftest.py and can be at different directory levels within a project directory structure to indicate the scope of the contained fixtures.

Is it possible to create an array or object setting to be used in the workspace's settings.json that takes a glob pattern to ignore a certain directory and/or filename?

For example, VS Code's seach.exclude setting is one of many examples using this.

The setting might look something like:

    "python.analysis.autoImportExclusions": {
        "conftest.py": true,
        "**/conftest.py": true,
        ...
    },

@jakebailey
Copy link
Member

Before we add any settings, we need to fix this completion context issue and not offer completions in conditions where we can't actually know a name. I wouldn't want to add a setting to work around an issue that we're going to end up fixing anyway (and that setting in particular is not as simple as you might think).

@WayneLambert
Copy link

OK, if PyLance doesn't add auto-imports for pytest fixtures once you've put in the fix, then I'm not worried how that's achieved. If a setting isn't required, then even better!

@jakebailey jakebailey changed the title Don't offer completions / auto-imports for class/function/parameter names Don't offer completions / auto-imports for class/function/parameter/import alias names Nov 4, 2020
@jakebailey jakebailey added fixed in next version (main) A fix has been implemented and will appear in an upcoming version and removed needs investigation Could be an issue - needs investigation labels Nov 10, 2020
@jakebailey
Copy link
Member

This issue has been fixed in version 2020.11.1, which we've just released. You can find the changelog here: https://github.com/microsoft/pylance-release/blob/master/CHANGELOG.md#2020111-11-november-2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed in next version (main) A fix has been implemented and will appear in an upcoming version
Projects
None yet
Development

No branches or pull requests

7 participants