Add support for `pathlib.Path`. Fix #170 #175

clintval · 2018-03-14T19:08:08Z

This closes #170. First time contributing so please let me know if this needs fixing! Thanks!

menshikh-iv · 2018-03-15T01:45:39Z

smart_open/smart_open_lib.py

@@ -163,6 +164,14 @@ def smart_open(uri, mode="rb", **kw):
    if not isinstance(mode, six.string_types):
        raise TypeError('mode should be a string')

+    # Support opening ``pathlib.Path`` objects by casting them to strings.
+    try:
+        from pathlib import Path


Oh, pathlib available only for python>=3.4, that's sad

Instead of checking for python>=3.4 and assuming pathlib support, I opted for the try-except pattern since there is a backport of pathlib that users may wish to use for python<3.4:

https://pypi.python.org/pypi/pathlib2/

Maybe add it to setup.py, wdyt @mpenkov? https://github.com/RaRe-Technologies/smart_open/blob/ecb24c23f4ad58904d371d88eeb1f83389cae16e/setup.py#L19-L23

menshikh-iv · 2018-03-15T01:48:51Z

smart_open/tests/test_smart_open.py

+            fpath = Path(os.path.join(CURR_DIR, 'test_data/cp852.tsv.txt'))
+            with smart_open.smart_open(fpath, encoding='cp852') as fin:
+                fin.read()
+        except ImportError:


That's a bad practice to silently pass the test, better to add a condition about python version for test https://docs.python.org/2/library/unittest.html#skipping-tests-and-expected-failures

clintval · 2018-03-15T18:57:48Z

Hi @menshikh-iv ready for another review.

Interestingly pathlib.Path was introduced in Python 3.4 but support for opening Path objects using the builtin open() was not introduced until Python 3.6. The test now supports these cases.

mpenkov · 2018-03-14T23:55:33Z

smart_open/tests/test_smart_open.py

+            from pathlib import Path
+            fpath = Path(os.path.join(CURR_DIR, 'test_data/cp852.tsv.txt'))
+            with smart_open.smart_open(fpath, encoding='cp852') as fin:
+                fin.read()


Don't you want to test for equality with the expected file content here?

Hi @mpenkov this commit is outdated. There is a more recent commit addressing review from @menshikh-iv.

Reviewed outdated commit. Newer commit addresses problem.

menshikh-iv · 2018-03-16T03:48:56Z

smart_open/tests/test_smart_open.py

@@ -191,6 +191,24 @@ def test_open_with_keywords_explicit_r(self):
            actual = fin.read()
        self.assertEqual(expected, actual)

+    @unittest.skipIf(
+        sys.version_info < (3, 4),


better to check that pathlib can be imported or not (not python version, because I can have installed pathlib from backport.

menshikh-iv · 2018-03-16T03:50:31Z

@mpenkov suspicious dependency conflict for 3.3 in CI https://travis-ci.org/RaRe-Technologies/smart_open/jobs/353985032, any ideas?

@clintval please merge the latest master into PR.

clintval · 2018-03-16T17:47:36Z

Ok @mpenkov and @menshikh-iv, I now skip the unit test if the module pathlib cannot be found or imported.

I rebased onto origin/master too.

Did you want me to add pathlib2 to test_require to test this library against the backport as well?

menshikh-iv · 2018-03-17T04:55:46Z

smart_open/tests/test_smart_open.py

@@ -191,6 +192,24 @@ def test_open_with_keywords_explicit_r(self):
            actual = fin.read()
        self.assertEqual(expected, actual)

+    @unittest.skipIf(
+        pkgutil.find_loader('pathlib') is None,


wow, I didn't know about pkgutil, @clintval 👍

menshikh-iv · 2018-03-17T04:58:41Z

@clintval yes, add it (and change skipping condition, because of pathlib and pathlib2 (2 different names, we need only one (any))

menshikh-iv · 2018-03-17T04:59:22Z

btw, really suspicious fail on Travis for 3.3, I'll investigate it later.

clintval · 2018-03-17T22:26:27Z

@menshikh-iv, opening pathlib.Path and pathlib2.Path instances is now supported!

The tests reflect this support.

See these test logs for how this PR acts without pathlib2 installed:
https://travis-ci.org/RaRe-Technologies/smart_open/builds/354849770

And these more recent logs for how this PR acts with pathlib2 installed:
https://travis-ci.org/RaRe-Technologies/smart_open/builds/354850402

The only unsupported condition is if you have both pathlib and pathlib2 backport installed, smart_open will only open paths from the builtin library and not from the backport. This is because:

>>> isinstance(pathlib.Path(), pathlib2.Path)
False

If you want concurrent pathlib and pathlib2 backport support let me know and I can modify the conditions (although this sounds like an awfully contrived scenario).

menshikh-iv · 2018-03-18T06:06:09Z

@clintval I'm +1 for using built-in pathlib even pathlib2 installed.

menshikh-iv · 2018-03-18T11:29:32Z

@clintval I resolved CI problem, now all fine
LGTM for me, wdyt @mpenkov?

mpenkov

I had a look at the commits. They look good, but I suggest a few minor changes to remove duplication of effort and simplify expression.

mpenkov · 2018-03-21T01:24:55Z

smart_open/tests/test_smart_open.py

+            # Unit test will skip if either module is unavailable so it's safe
+            # to assume we can import _at least_ one working ``pathlib``.
+            except ImportError:
+                pass

        # builtin open() supports pathlib.Path in python>=3.6 only


I don't think we need to bother with this is in the tests. From the point of view of the test, it's an unnecessary detail. In this particular case, we just want to read the expected data in the quickest and simplest possible way. So in this case, I think it's better to just do:

with open(fpath) as fin: expected = fin.read().decode('cp852')

mpenkov · 2018-03-21T01:31:52Z

smart_open/tests/test_smart_open.py

@@ -193,16 +194,28 @@ def test_open_with_keywords_explicit_r(self):
        self.assertEqual(expected, actual)

    @unittest.skipIf(


I think a simpler way of writing this is:

@unittest.skipIf(smart_open_lib.PATHLIB_SUPPORT is False, "your reason here")

mpenkov · 2018-03-21T01:35:11Z

smart_open/tests/test_smart_open.py

+        """If ``pathlib.Path`` is available we should be able to open and read."""
+        fpath = os.path.join(CURR_DIR, 'test_data/cp852.tsv.txt')
+
+        # Import ``pathlib`` if the builtin ``pathlib`` or the backport


I think we're duplicating unnecessary work here. We've already gone through the pain of importing the necessary pathlib module in smart_open_lib. So, we can just get rid of all this import stuff, and use:

from smart_open.smart_open_lib import pathlib path = pathlib.Path('/foo/bar')

whenever you need access to the actual pathlib module.

…s==39.0.0`. Fix #176 (#178) * fix author/maintainer fields * attemp to resolve problem with botocore * try other workaround for botocore * next attemp

clintval · 2018-03-21T04:58:18Z

Thanks @mpenkov! Great review.

I completely agree and pushed another commit slimming the test down.

Here are the Path instances we support opening:

Python Version	`pathlib2.Path`	`pathlib.Path`
<3.4	✔
≥3.4		✔

mpenkov

Looks like my previous comment wasn't clear enough, so let me clarify here.

mpenkov · 2018-03-21T05:47:30Z

smart_open/tests/test_smart_open.py

            actual = fin.read()
-        self.assertEqual(expected, actual)


We still want to check that the contents are equal. We just don't want to read the expected file via open/pathlib, because that won't work for all Python versions, as you've correctly pointed out.

So the correct thing to do here is (untested pseudo-code):

expected = open(fpath).read().decode('cp852') self.assertEqual(expected, actual)

You can use a with statement if you like, I guess that's more Pythonic.

I'll add that back in, I misunderstood your previous comment.

@mpenkov thanks for your patience. I amended the commit.

mpenkov · 2018-03-22T03:59:37Z

@menshikh-iv LGTM, please merge when you're ready.

menshikh-iv · 2018-03-22T06:24:39Z

Thanks @clintval 👍, good work!

clintval force-pushed the cv_issue_170 branch from b9d4d76 to 60236b7 Compare March 14, 2018 19:14

Support opening pathlib.Path

60236b7

menshikh-iv suggested changes Mar 15, 2018

View reviewed changes

clintval force-pushed the cv_issue_170 branch 5 times, most recently from b295f04 to dacbe67 Compare March 15, 2018 18:53

mpenkov previously requested changes Mar 16, 2018

View reviewed changes

menshikh-iv reviewed Mar 16, 2018

View reviewed changes

clintval force-pushed the cv_issue_170 branch 3 times, most recently from d0df9b1 to 9c57e82 Compare March 16, 2018 17:42

Edited opening pathlib.Path test which skips if not available

9c57e82

menshikh-iv reviewed Mar 17, 2018

View reviewed changes

clintval force-pushed the cv_issue_170 branch 2 times, most recently from 5cbc331 to d9f60b5 Compare March 17, 2018 22:15

clintval added 2 commits March 17, 2018 15:16

Added pathlib2 backport support including tests

d9f60b5

Added pathlib2 to test_require for supporting pathlib backport

7b88cb1

menshikh-iv mentioned this pull request Mar 18, 2018

Fix author/maintainer fields in setup.py, avoid bug from setuptools==39.0.0. Fix #176 #178

Merged

mpenkov requested changes Mar 21, 2018

View reviewed changes

clintval force-pushed the cv_issue_170 branch from aa6680e to 1a33339 Compare March 21, 2018 01:54

menshikh-iv added 3 commits March 20, 2018 18:54

Fix author/maintainer fields in setup.py, avoid bug from `setuptool…

bfa729c

…s==39.0.0`. Fix #176 (#178) * fix author/maintainer fields * attemp to resolve problem with botocore * try other workaround for botocore * next attemp

bump version to 1.5.7

ae23fa4

bump changelos to 1.5.7

279b4d7

clintval force-pushed the cv_issue_170 branch 4 times, most recently from 7483fb9 to 452b194 Compare March 21, 2018 02:06

mpenkov requested changes Mar 21, 2018

View reviewed changes

clintval force-pushed the cv_issue_170 branch from 452b194 to 9f0d342 Compare March 21, 2018 18:50

Reduced code logic redundancy in pathlib.Path test

9f0d342

mpenkov approved these changes Mar 22, 2018

View reviewed changes

Merge remote-tracking branch 'upstream/master' into cv_issue_170

94e903a

menshikh-iv approved these changes Mar 22, 2018

View reviewed changes

menshikh-iv changed the title ~~Support opening pathlib.Path~~ Add support for pathlib.Path. Fix #170 Mar 22, 2018

menshikh-iv merged commit 0033cb9 into piskvorky:master Mar 22, 2018

clintval deleted the cv_issue_170 branch March 22, 2018 18:17

menshikh-iv mentioned this pull request Apr 1, 2018

Compatibility issue with pathlib piskvorky/gensim#2010

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `pathlib.Path`. Fix #170 #175

Add support for `pathlib.Path`. Fix #170 #175

clintval commented Mar 14, 2018

menshikh-iv Mar 15, 2018

clintval Mar 15, 2018 •

edited

Loading

menshikh-iv Mar 16, 2018 •

edited

Loading

menshikh-iv Mar 15, 2018

clintval commented Mar 15, 2018 •

edited

Loading

mpenkov Mar 14, 2018

clintval Mar 16, 2018

menshikh-iv Mar 16, 2018

menshikh-iv commented Mar 16, 2018

clintval commented Mar 16, 2018 •

edited

Loading

menshikh-iv Mar 17, 2018 •

edited

Loading

menshikh-iv commented Mar 17, 2018

menshikh-iv commented Mar 17, 2018

clintval commented Mar 17, 2018 •

edited

Loading

menshikh-iv commented Mar 18, 2018

menshikh-iv commented Mar 18, 2018

mpenkov left a comment

mpenkov Mar 21, 2018

mpenkov Mar 21, 2018

mpenkov Mar 21, 2018

clintval commented Mar 21, 2018 •

edited

Loading

mpenkov left a comment

mpenkov Mar 21, 2018

clintval Mar 21, 2018

clintval Mar 21, 2018

mpenkov commented Mar 22, 2018

menshikh-iv commented Mar 22, 2018

		@@ -193,16 +194,28 @@ def test_open_with_keywords_explicit_r(self):
		self.assertEqual(expected, actual)

		@unittest.skipIf(

Add support for pathlib.Path. Fix #170 #175

Add support for pathlib.Path. Fix #170 #175

Conversation

clintval commented Mar 14, 2018

Choose a reason for hiding this comment

clintval Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

menshikh-iv Mar 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintval commented Mar 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

menshikh-iv commented Mar 16, 2018

clintval commented Mar 16, 2018 • edited Loading

menshikh-iv Mar 17, 2018 • edited Loading

Choose a reason for hiding this comment

menshikh-iv commented Mar 17, 2018

menshikh-iv commented Mar 17, 2018

clintval commented Mar 17, 2018 • edited Loading

menshikh-iv commented Mar 18, 2018

menshikh-iv commented Mar 18, 2018

mpenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintval commented Mar 21, 2018 • edited Loading

mpenkov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpenkov commented Mar 22, 2018

menshikh-iv commented Mar 22, 2018

Add support for `pathlib.Path`. Fix #170 #175

Add support for `pathlib.Path`. Fix #170 #175

clintval Mar 15, 2018 •

edited

Loading

menshikh-iv Mar 16, 2018 •

edited

Loading

clintval commented Mar 15, 2018 •

edited

Loading

clintval commented Mar 16, 2018 •

edited

Loading

menshikh-iv Mar 17, 2018 •

edited

Loading

clintval commented Mar 17, 2018 •

edited

Loading

clintval commented Mar 21, 2018 •

edited

Loading