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

Publish MonkeyPatch for typing #7453

Closed
wants to merge 1 commit into from

Conversation

dirn
Copy link
Contributor

@dirn dirn commented Jul 6, 2020

This builds on the work done in 2bcad38
to add MonkeyPatch to pytest's public API. This will allow users of
pytest to annotation their tests like

def test_some_code(monkeypatch: MonkeyPatch) -> None:
    ...

To make this possible, the monkeypatch module can't import pytest
anymore. Instead it will import PytestWarning directly from
_pytest.warning_types.

@dirn dirn marked this pull request as draft July 6, 2020 13:57
@dirn dirn force-pushed the monkeypatch-as-public-type branch from 3893521 to b6363de Compare July 6, 2020 14:02
This builds on the work done in 2bcad38
to add `MonkeyPatch` to pytest's public API. This will allow users of
pytest to annotation their tests like

    def test_some_code(monkeypatch: MonkeyPatch) -> None:
        ...

To make this possible, the `monkeypatch` module can't import `pytest`
anymore. Instead it will import `PytestWarning` directly from
`_pytest.warning_types`.
@dirn dirn force-pushed the monkeypatch-as-public-type branch from b6363de to 20e7a5b Compare July 6, 2020 14:04
@dirn dirn marked this pull request as ready for review July 6, 2020 14:12
@nicoddemus
Copy link
Member

Hmm unfortunately this also makes the MonkeyPatch class public, which might be a problem because it can prevent a refactoring (however for MonkeyPatch itself this seems unlikely).

Is there another way to expose the typing API of the class without exposing the actual implementation?

@dirn
Copy link
Contributor Author

dirn commented Jul 7, 2020

You could make a protocol somewhere in pytest that matches MonkeyPatch (you could even use the same name), but then you'd have two things you'd need to update anytime you want to change the API of MonkeyPatch.

@bluetech
Copy link
Member

bluetech commented Jul 7, 2020

Thanks for bringing this up @dirn. I had been planning to create an issue about this but haven't got to it yet.

The issue is more widespread than MonkeyPatch, it is relevant to all of pytest's fixtures, as well as some other types. As @nicoddemus said, we don't want to straight-out expose them because that will be interpreted as being completely public/stable API.

Your suggestion to use a Protocol is something we should consider, although it will be somewhat unwieldy and has some backward compat issues (we still support Python 3.5).

With most of these types, the non-underscored methods and and attributes are meant to be public, but the constructor is not, and they are also not designed for subclassing. In these cases we can prevent direct instantiation and subclassing by copying what trio does: https://github.com/python-trio/trio/blob/75febf1f0519532631901a340bcfc67b0c948616/trio/_util.py#L290-L356

In any case I plan to open an issue listing all of the relevant types and seeing what kind of privacy they require, and then we can come up with a plan. It's possible it won't be ready for 6.0.0 however.

@nicoddemus
Copy link
Member

nicoddemus commented Jul 7, 2020

(Thanks @bluetech for the nice summary, I was in a bit of a hurry and couldn't elaborate further).

Couldn't we instead define an ABC for each pytest fixture result and use that, at least until we can use Protocol in all versions supported by pytest? Of course this means subclassing from them, but I don't see that as a problem. Anyway we can discuss this approach on the issue @bluetech plans to create. 😁

I'm closing this for now, but thanks @dirn, we appreciate the PR nonethless!

@nicoddemus nicoddemus closed this Jul 7, 2020
@dirn
Copy link
Contributor Author

dirn commented Jul 7, 2020

I thought about suggesting an ABC as well. I don’t know enough about how the fixtures are implemented to know if that would make sense or not. I’m glad to see this is on your radar.

@bluetech
Copy link
Member

bluetech commented Jul 9, 2020

I created the issue: #7469. Let me know what you think there!

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