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

Add support for pathlib2 (#408) #422

Merged
merged 8 commits into from Sep 3, 2018

Conversation

Projects
None yet
3 participants
@matthew16550
Copy link
Contributor

matthew16550 commented Aug 7, 2018

No description provided.

@@ -0,0 +1,19 @@
import sys

This comment has been minimized.

@matthew16550

matthew16550 Aug 7, 2018

Author Contributor

I'm not very happy with the approach in this file but don't have a better idea.

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

An alternative would be to not install pathlib2 and scandir in the first place, run the tests, install the packages and run the tests again.

This comment has been minimized.

@matthew16550

matthew16550 Aug 8, 2018

Author Contributor

Unfortunately pytest depends on pathlib2 which depends on scandir!

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 8, 2018

Collaborator

Hm, why is that? pytest itself seems to depend only on py.

This comment has been minimized.

@matthew16550

matthew16550 Aug 8, 2018

Author Contributor

Aha I see what's happening, their setup.py adds pathlib2 as a dependency when Python < 3.6 so if we find no other approach I'll update the comment in extra_requirements.txt.

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 8, 2018

Collaborator

Ah, I see - well, I can't think of another approach right now in this case. Maybe @jmcgeheeiv has an idea, otherwise I would it leave as is.

@@ -496,10 +504,10 @@ def open(self, mode='r', buffering=-1, encoding=None,
"""
if self._closed:
self._raise_closed()
return FakeFileOpen(self.filesystem)(
return FakeFileOpen(self.filesystem, use_io=True)(

This comment has been minimized.

@matthew16550

matthew16550 Aug 7, 2018

Author Contributor

I added use_io=True 3 times in this file to make some py27 tests pass on Windows. Not sure if it's the best approach, maybe I should add something to the self._use_io logic in FakeFileOpen.__init__() instead?

The test failures can be seen in AppVeyor, e.g:

Traceback (most recent call last):
  File "pyfakefs\tests\fake_filesystem_unittest_test.py", line 131, in test_fakepathlib
  File "pyfakefs\fake_pathlib.py", line 508, in open
  File "pyfakefs\fake_filesystem.py", line 4765, in __call__
TypeError: _call_ver2() takes at most 6 arguments (7 given)
Traceback (most recent call last):
  File "pyfakefs\tests\fake_pathlib_test.py", line 508, in test_read_text
    self.assertEqual(file_path.read_text(), 'foo')
  File "pyfakefs\fake_pathlib.py", line 527, in read_text
    errors=errors) as f:
  File "pyfakefs\fake_filesystem.py", line 4765, in __call__
    return self._call_ver2(*args, **kwargs)
TypeError: _call_ver2() got an unexpected keyword argument 'errors'

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

I think adding use_io=True is correct - this was not needed before because it is the default in Python3 (os.open is an alias to io.open there). Obviously, pathlib2 uses io.open() both in Python 2 and 3 for compatibility reasons. I haven't checked yet why this fails only under Windows, but I guess the Windows implementation uses an io-specific argument in the open call.

path = self.path('/foo/bar')
self.assertRaises(FileNotFoundError, path.resolve)

This comment has been minimized.

@jmcgeheeiv

jmcgeheeiv Aug 7, 2018

Owner

I like these little cleanups. Well done.

@mrbean-bremen
Copy link
Collaborator

mrbean-bremen left a comment

Very nice work! Especially I liked how you integrated that with the scanlib support. I have to admit that I don't use pathlib myself, and added the support purely based on the Python documentation and source code, so I'm glad to see that it is really used.
There are only a few changes I would like to see, nothing really important.

@@ -0,0 +1,19 @@
import sys

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

An alternative would be to not install pathlib2 and scandir in the first place, run the tests, install the packages and run the tests again.

@@ -60,10 +61,8 @@ def test_shutil_patch(self):
self.fs.set_disk_usage(100)
self.assertEqual(100, shutil.disk_usage('/').total)

@unittest.skipIf(sys.version_info < (3, 4), 'pathlib new in Python 3.4')
@unittest.skipIf(not pathlib, 'pathlib new in Python 3.4')

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

You may want to adapt the comments (e.g. something like "run only if pathlib is available").

use_scandir_package = True
except ImportError:
use_scandir = False
use_scandir_package = False

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

I like this approach with the extra packages! The integration of scandir was a bit of a hack, so this is much better.

@@ -496,10 +504,10 @@ def open(self, mode='r', buffering=-1, encoding=None,
"""
if self._closed:
self._raise_closed()
return FakeFileOpen(self.filesystem)(
return FakeFileOpen(self.filesystem, use_io=True)(

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

I think adding use_io=True is correct - this was not needed before because it is the default in Python3 (os.open is an alias to io.open there). Obviously, pathlib2 uses io.open() both in Python 2 and 3 for compatibility reasons. I haven't checked yet why this fails only under Windows, but I guess the Windows implementation uses an io-specific argument in the open call.

text_type = unicode # Python 2
except NameError:
text_type = str # Python 3

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

I actually wanted to move this one out before, but forgot it - nice job!

self.assert_raises_os_error(
errno.EEXIST, callableObj, *args, **kwargs)
else:
self.assertRaises(FileExistsError, callableObj, *args, **kwargs)

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

I think this is not needed - FileExistsError behaves like OSError with errno.EEXIST, so the check for that one is enough.

self.assert_raises_os_error(
errno.ENOENT, callableObj, *args, **kwargs)
else:
self.assertRaises(FileNotFoundError, callableObj, *args, **kwargs)

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

Same as above - just use self.assert_raises_os_error directly.


is_windows = sys.platform == 'win32'


def needs_pathlib_35():
if sys.version_info < (3, 5) and not pathlib2:
raise unittest.SkipTest('New in version 3.5')

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

Nice - though I would rename these to be consistent with the other skip functions (e.g. either skip_if_xxx or check_if_xxx).

@@ -467,35 +498,53 @@ def test_exists(self):

def test_open(self):
self.create_dir(self.make_path('foo'))
self.assertRaises(OSError,
self.assertRaises(IOError if TestCase.is_python2 else OSError,

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

IOError is an alias for OSError in Python 3, so you may just check for IOError.

pytest>=2.8.6
scandir>=1.8

This comment has been minimized.

@mrbean-bremen

mrbean-bremen Aug 7, 2018

Collaborator

I would prefer to leave this file as is, as it contains the requirements needed for pyfakefs, and put the extra requirements into an extra requirements file.

matthew16550 added some commits Aug 8, 2018

Address PR comments
Add all_tests_without_extra_packages to AppVeyor run
Doh
@mrbean-bremen

This comment has been minimized.

Copy link
Collaborator

mrbean-bremen commented Aug 8, 2018

As for the failing test in TestCopyOrAddRealFile - this is most likely due to __file__ pointing to the compiled (pyc) file instead of the source file on the second run. This seems to happen under Windows / Python 2.7 and has to be fixed in the setup of that test class.

@matthew16550

This comment has been minimized.

Copy link
Contributor Author

matthew16550 commented Aug 8, 2018

Yep that fixed TestCopyOrAddRealFilethanks.

I've made all the changes you suggested so the only thing left is a final decision on all_tests_without_extra_packages.py.

@mrbean-bremen

This comment has been minimized.

Copy link
Collaborator

mrbean-bremen commented Aug 9, 2018

Looks fine to me - thanks a lot! @jmcgeheeiv will have a second review and handle the merge.

@jmcgeheeiv jmcgeheeiv merged commit 05e7d2a into jmcgeheeiv:master Sep 3, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

jmcgeheeiv added a commit that referenced this pull request Sep 3, 2018

@mrbean-bremen mrbean-bremen referenced this pull request Sep 3, 2018

Closed

Support pathlib2 #408

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.