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.

Sign up for GitHub

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

[wip] Future/pathlib - make used path type configurable #2230

Closed
wants to merge 2 commits into from
Closed

[wip] Future/pathlib - make used path type configurable #2230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2656785
introduce config.make_path - beginning of supporting other path imple…
RonnyPfannschmidt Feb 3, 2017
5289ca6
add comment wrt disabled core plugin
RonnyPfannschmidt Feb 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 24 additions & 4 deletions _pytest/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -972,11 +972,31 @@ def _initini(self, args):
ns, unknown_args = self._parser.parse_known_and_unknown_args(args, namespace=self.option.copy())
r = determine_setup(ns.inifilename, ns.file_or_dir + unknown_args, warnfunc=self.warn)
self.rootdir, self.inifile, self.inicfg = r
self._parser.extra_info['rootdir'] = self.rootdir
self._parser.extra_info['inifile'] = self.inifile
self.invocation_dir = py.path.local()
self._parser.addini('addopts', 'extra command line options', 'args')
self._parser.addini('minversion', 'minimally required pytest version')
self._parser.addini('pathtype', 'path implementation to be used', default='pylib')

self.invocation_dir = self.make_path()
self.rootdir = self.make_path(self.rootdir)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately this would break all plugins which use this; then users are in the unfortunate situation where they can't make use of this feature because it breaks some of their plugins, and plugin authors will have to support both APIs in order to satisfy all users...

On the other hand I don't have any idea on how to solve this. 😞

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why this will be and opt-in and will support a transitional api for quite a while

however py.path has to die

the starting point in any case is compatibility wrappers and the ability to switch
only after all major plugins support the transition apis we can take a look at switching the default

this is something that will have a transition period way beyond the lifetime of python 2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is why this will be and opt-in and will support a transitional api for quite a while
this is something that will have a transition period way beyond the lifetime of python 2

Oh OK that seems sensible, and as soon as we add an alternative the better. But what do you mean by a "transitional API" exactly? What you did here or something else entirely like a py.local subclass which supports pathlib.Path` methods?

however py.path has to die

Yeah, it had its purpose of course but with pathlib on standard library it doesn't make sense to maintain it in the long run.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didnt even start with a transitional api - whats in here right now is just the opt in to change
my next steps wil lbe to experiment wit the transition

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it, thanks

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be simpler to add a new fixture and deprecate the tmpdir one? How many other places is py.path.local exposed?
So I assume you also looked at making py.path.local follow the __fspath__ protocol from PEP 519 and that it's not sanely doable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How many other places is py.path.local exposed?

Unfortunately in quite a lot of places, for example the fspath attribute of test items, config.rootdir and config.inifile come to mind.

So I assume you also looked at making py.path.local follow the fspath protocol from PEP 519 and that it's not sanely doable?

That's an interesting idea, although I'm not sure if that would help us reach the ultimate goal which is to drop py.path.local altogether... at least that's what I thought the ultimate goal was.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fspath is already supported, but the api differs

self.inifile = self.make_path(self.inifile)
self._parser.extra_info['rootdir'] = self.rootdir
self._parser.extra_info['inifile'] = self.inifile

def make_path(self, input=None):
if input is None:
input = os.getcwd()
pathtype = self.getini('pathtype')
if pathtype == 'pylib':
return py.path.local(input)
elif pathtype == 'pathlib':
# for pythons that dont use fspath
if isinstance(input, py.path.local):
input = str(input)
import pathlib
return pathlib.Path(input)
elif pathtype == 'pathlib2':
import pathlib2
return pathlib2.Path(input)

def _consider_importhook(self, args, entrypoint_name):
"""Install the PEP 302 import hook if using assertion re-writing.
Expand Down Expand Up @@ -1311,7 +1331,7 @@ def determine_setup(inifile, args, warnfunc=None):
if rootdir is None:
rootdir = get_common_ancestor([py.path.local(), ancestor])
is_fs_root = os.path.splitdrive(str(rootdir))[1] == os.sep
if is_fs_root:
if is_fs_root:
rootdir = ancestor
return rootdir, inifile, inicfg or {}

Expand Down
34 changes: 34 additions & 0 deletions testing/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,23 @@
from _pytest.config import getcfg, get_common_ancestor, determine_setup
from _pytest.main import EXIT_NOTESTSCOLLECTED

try:
from pathlib import Path as PathlibPath
except ImportError:
PathlibPath = None

try:
from pathlib2 import Path as Pathlib2Path
except ImportError:
Pathlib2Path = None

PATHIMPLS = {
'pylib': py.path.local,
'pathlib': PathlibPath,
'pathlib2': Pathlib2Path,
}


class TestParseIni:

@pytest.mark.parametrize('section, filename',
Expand Down Expand Up @@ -73,6 +90,20 @@ def test_toxini_before_lower_pytestini(self, testdir):
config = testdir.parseconfigure(sub)
assert config.getini("minversion") == "2.0"

@pytest.mark.parametrize('pathimpl, pathtype', sorted(PATHIMPLS.items()))
def test_configure_makepath(self, testdir, pathimpl, pathtype):
if pathtype is None:
pytest.skip(
"{} path implementation is missing, not testing"
.format(pathimpl))
testdir.makeini("""
[pytest]
pathtype = {}
""".format(pathimpl))
# disable cacheprovider to get out of breaking bad
config = testdir.parseconfigure('-p', 'no:cacheprovider')
assert isinstance(config.make_path(testdir.tmpdir), pathtype)

@pytest.mark.xfail(reason="probably not needed")
def test_confcutdir(self, testdir):
sub = testdir.mkdir("sub")
Expand Down Expand Up @@ -809,4 +840,7 @@ def test_with_existing_file_in_subdir(self, tmpdir):
rootdir, inifile, inicfg = determine_setup(None, ['a/exist'])
assert rootdir == tmpdir
assert inifile is None