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

Support typing of fixture parameters #28

Merged
merged 5 commits into from
Oct 26, 2022
Merged

Conversation

thu2004
Copy link
Contributor

@thu2004 thu2004 commented Oct 13, 2022

src/fixtureParser.ts

  • correctly locate pytest fixture when they are install with pip
  • Add returnType and module attribute to Fixture

src/suggestionProvider.ts

  • support typing of fixture parameter
  • Change completion item kind from Field to Snippet together with the snippet setting, show our suggestions on the top of the suggestion list

Add 2 more tests for typing of fixture parameters

Copy link
Owner

@nickjmiller nickjmiller 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 doing this!

src/fixtureParser.ts Outdated Show resolved Hide resolved
src/fixtureParser.ts Outdated Show resolved Hide resolved
@thu2004
Copy link
Contributor Author

thu2004 commented Oct 19, 2022

@nickjmiller please release it if the feature is approved.

@nickjmiller
Copy link
Owner

The new tests don't seem to be passing on a windows machine and the functionality isn't working either. I can look into it a little deeper this weekend, it may just be related to the path separator.

Chi Thu Le added 2 commits October 22, 2022 21:12
Now tests are independence other installed plugins
@thu2004
Copy link
Contributor Author

thu2004 commented Oct 22, 2022

I improved tests and test data. It is now independent what other pytest plugins your have in your system. Instead of compare if the fixture list is match, now it checks if the expected fixtures exist in the fixture list reported by pytest.

I downloaded and start a Windows virtual machine and run all tests successfully both on Windows and Linux OS.

@nickjmiller
Copy link
Owner

Thanks, the tests are working now and the functionality as well, I just didn't read the README close enough before.

I do think it's a bit unintuitive to trigger this, what do you think of including the return type in the original CompletionItem, e.g.:

let fixtureName = fixture.name;
if (fixture.returnType) {
    fixtureName += `: ${fixture.returnType}`;
}
let item = new vscode.CompletionItem(
    fixtureName,
    vscode.CompletionItemKind.Snippet
);
...

I think this might be a nice way to expose this, but it would need to be a toggleable option since earlier versions of python wouldn't support it and some people might not want typing. Or maybe two suggestions could be provided:

let fixtureName = fixture.name;
let item = new vscode.CompletionItem(
    fixtureName,
    vscode.CompletionItemKind.Snippet
);
if (fixture.returnType) {
    let itemWithType = new vscode.CompletionItem(
        fixtureName: `${fixtureName}: ${fixture.returnType}`,
        vscode.CompletionItemKind.Snippet
    );
...

@thu2004
Copy link
Contributor Author

thu2004 commented Oct 25, 2022

I started with the solution to autocomplete fixture with the type as you showed above. I found some drawbacks and it is not intuitive.

  • The need of a new setting
  • tester does not care about all fixtures with typing, for example some of the pytest build-in fixture:
    def test_aaa(capsysbinary:Generator[CaptureFixture[bytes], None, None])
    def test_aaa(monkeypatch:Generator["MonkeyPatch", None, None])
    It a pain to resolve all needed import statements or the user have to manually remove the typing.
    Please try the above fixture.
  • It is intuitive to use ':' and then autocomplete if you think outside the fixture context.
    When you define a new function with parameter, after you typed the name of parameter and if you want to add typing, you press ':' then write the type or trigger autocomplete to get suggestion of type.

@nickjmiller nickjmiller merged commit 56ff675 into nickjmiller:main Oct 26, 2022
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