de-emphasize request.addfinalizer Fixes #5587

[graingert]

• Target the master branch for bug fixes, documentation updates and trivial changes.
• Target the features branch for new features and removals/deprecations.
• Include documentation when adding new features.
• Include new tests or update existing tests when applicable.

Unless your change is trivial or a small documentation fix (e.g., a typo or reword of a small section) please:

• Create a new changelog file in the changelog folder, with a name like <ISSUE NUMBER>.<TYPE>.rst. See changelog/README.rst for details.
• Add yourself to AUTHORS in alphabetical order;

Fixes #5587

[codecov]

Codecov Report

Merging #5589 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5589      +/-   ##
==========================================
- Coverage   96.12%   96.11%   -0.01%     
==========================================
  Files         117      117              
  Lines       25733    25775      +42     
  Branches     2493     2495       +2     
==========================================
+ Hits        24735    24773      +38     
- Misses        695      697       +2     
- Partials      303      305       +2

Impacted Files	Coverage Δ
src/_pytest/pathlib.py	87.89% <0%> (-1.19%)	⬇️
testing/test_tmpdir.py	98.52% <0%> (+0.21%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2c402f4...a96710d. Read the comment docs.

[graingert]

I'm not keen on deprecating/removing addfinalizer though. Are there usecases for addfinalizer that ExitStack cannot provide?

[RonnyPfannschmidt]

addfinalizer can be implemented in terms of adding a callback to a stack

it would be interesting if there was a per usage scope fixture for eexit stacks,

then each fixture could get a very own exit stack in a well expressed manner (and if each fixture had one, it wouldnt be as bad to expose it)

[nicoddemus]

I'm not sure this is worth adding at all, request.addfinalizer is actually more concise:

    @pytest.fixture
    def equipments(request):        
        r = []
        for port in ("C1", "C3", "C28"):
            equip = connect(port)
            request.addfinalizer(equip.disconnect)
            r.append(equip)
        yield r

Given that we definitely won't deprecate request.addfinalizer, I don't see much value in adding this particular example TBH. 🤔

[graingert]

It's more concise in this particular case but it doesn't let you use layered resources or mix contexts and callbacks.

ExitStack would be more concise considering realistically connect will return a context manager

[nicoddemus]

ExitStack would be more concise considering realistically connect will return a context manager

Can you update the example to make use of this then? I believe it would then make more sense.

[graingert]

done

[nicoddemus]

Please update the example so it uses with statement for the connections too; this way the example will make more sense as an alternative to addfinalizer. 👍

Thanks!

[graingert]

🤢 now this is why ExitStack is so great

[nicoddemus]

Oh yeah, for context managers then ExitStack is definitely the superior alternative. Do you think it is worth it thought to add this example like this? I'm afraid users might get confused by it.

[graingert]

I'm tempted to remove docs for addfinalizer entirely

[graingert]

I had a go at removing all pytest's own calls to .addfinalizer and replacing them with plain yield fixtures, and replacing it with either layered resource acquisition or an ExitStack. And it looks much nicer.

It's quite a big change, doesn't really gain anything, and I got bored half way and gave up - but it just demonstrates the superiority of the yield fixture + ExitStack approach.

If we actually want to go ahead and deprecate addfinalizer then it would obviously be worth removing our own uses of it

[nicoddemus]

I disagree, it is part of the official API and should be in the docs, even if it is not the best alternative for all cases. People might be maintaining old test suites which use addfinalizer, and they might need the docs.

[The-Compiler]

FWIW request.addfinalizer is also still (AFAIK) the only way to do cleanups from inside a test (rather than a fixture).

[graingert]

def test_something():
    with contextlib.ExitStack as stack:
        setup_something()
        stack.callback(cleanup_something)
        assert sut()

[graingert]

@nicoddemus should I hit "Merge pull request" ?

[nicoddemus]

Sure, go ahead! 👍