Add dot prefix if file makefile extension is invalid for pathlib

[bengartner]

Closes #8192

[bluetech]

Thanks for looking at this!

The change currently adds a warning to pytester. I think what we want rather is:

1. Change testdir.makefile to silently add the ., to keep backward compat. testdir is going to be deprecated in its entirety sometime in the future so I wouldn't bother with something else for it.

2. Change pytester._makefile to raise a more informative error when the . is missing (currently it fails in some pathlib error). This way the new pytester enforces the correct non-confusing usage.

[bengartner]

Thanks for clarifying - I assumed py.path.local was being dropped and didn't realize the testdir fixture is going to coexist with pytester. I'm new here and haven't worked with either fixture before - or warnings or py.path.

If I understand correctly, pytester is recommended for new code, but testdir is still supported - pytest is not going so far as to even deprecate it yet. Given that, it makes sense that testdir should not start bothering users about something as trivial as this edge cause in file creation. If testdir gets deprecated in the future, it can just throw its own deprecation warning.

I added a commit does the most naive thing: it catches and ignores the warning when using testdir. The makefile function wraps pytester so the Path object is created, then converted, so there's not an easy way for pytester to be noisier than testdir. However, this could easily squelch other warnings we don't want to ignore, so it may make sense to subtype the Warning, find some other way to be more selective, or use a different strategy entirely.

2. Change pytester._makefile to raise a more informative error when the . is missing (currently it fails in some pathlib error). This way the new pytester enforces the correct non-confusing usage.

To clarify, you're asking not for a more informative warning than in my original commit, but to catch the pathlib exception and raise an exception with stronger enforcement than the warning? This seems sensible and doable.

However, I think @RonnyPfannschmidt had a warning in mind for an ext argument in testdir where ext[0] != ".", since that is almost certainly user error. This is based on the #8222 discussion. I guess the question is, a) do we want to raise a warning that py.path should probably be raising themselves and b) do we want to start now, given that testdir is slowly evaporating anyway?

So two points where I don't have strong opinions so looking for guidance/consensus:

1. Does the code ignoring the warning need to be improved or is it good enough?
2. Should we upgrade the pytester warning I just added to an exception?

[bluetech]

I'm new here and haven't worked with either fixture before - or warnings or py.path.

First of all, welcome :)

The following is how I see things, let me know if you agree. In the current state (before this PR) IMO we have 2 issues:

1. testdir.makefile('xxx') (no dot in the first argument) used to work, now it fails. So we broke backward compat (or rather bug compat 😀)
2. When a dotless value is passed to pytester.makefile, an unintelligible pathlib exception is thrown instead of something more friendly.

The way I propose to fix it is:

1. In testdir._makefile, if receiving a dotless value, just add it silently (in testdir itself). This preserves compat for testdir and testdir only. Do it silently because new users shouldn't be using testdir anyway, and old users probably just want it to stay the same.
2. In pytester.makefile, if receiving a dotless value, raise a ValueError with a nice error message explaining the proper usage.

Note that in this solution there are no warnings or catching warnings, just a silent fix in testdir and a more friendly error in pytester.

[bengartner]

Okay, I now see the easy way to do this that I previously claimed did not exist. Thank you for loosening the scales covering my eyes.

I rebased to a simple commit that should avoid the pathlib ValueError in both cases. Will be silent for testdir with the workaround discussed. And for pytester, we throw our own ValueError beforehand. Let me know if you think there's still a py.test edge case we want to cover here, I saw the pathlib syntax is if suffix and not suffix.startswith('.') or suffix == '.': but not sure what py.path will do in if suffix='.' or suffix is Falsey. Hopefully we don't want to maintain compatibility with testdir.makefile('.')?

[bengartner]

As it happens, coverage wanted to see the not ext condition covered, so I dug up the behavior on tag 5.0.0 and added tests to get coverage and verify the same behavior. Is there a reason to backport these, as they are essentially regression tests?

[bluetech]

LGTM! Just need an update to the changelog entry to match the latest iteration (I left a suggestion).