Fido.fetch now accepts pathlib.Path objects #2559

vn-ki · 2018-03-13T11:59:13Z

Closes #2400.

pep8speaks · 2018-03-13T11:59:15Z

Hello @vn-ki! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on March 21, 2018 at 20:18 Hours UTC

nabobalis · 2018-03-13T12:03:25Z

sunpy/net/dataretriever/client.py

@@ -336,6 +337,15 @@ def fetch(self, qres, path=None, error_callback=None, **kwargs):
        -------
        Results Object
        """
+        # Check for type of path
+        if isinstance(path, str) or path is None:
+            pass


Do we need this if statement?

Also I wonder why the documentation string for this is empty of the keywords.

This is to ensure that the path which is None, str or pathlib.Path goes past. Any other type would raise a TypeError

Could the if,elif,else block be refactored to avoid having the if:pass part?

I don't see how we can go past not checking whether path is str or None.

If you have some suggestion on how to do this, can you share?

@Cadair might

I haven't tested this, so it probably has several syntax issues, but something like this maybe:

if path is not None: if isinstance(path, pathlib.Path) and HAS_PATHLIB: path = str(path.absolute()) elif not isinstance(path, str): # need py2 fix for six here? raise TypeError("path keyword is invalid. Should be a 'str' or 'pathlib.Path'.\ Got '{}'.".format(type(path)))

I think if we check for str we need to use six to ensure python 2 compatibility.

nabobalis · 2018-03-13T12:07:03Z

sunpy/net/dataretriever/tests/test_eve.py

+    assert len(response1) == qr._numfile
+    path2 = pathlib.Path('.')
+    response2 = Fido.fetch(qr, path=path2)
+    assert len(response2) == qr._numfile


I wonder if Fido.fetch doesn't already have tests for this somewhere else. Not sure that this should be added just under an eve test.

Sorry for not looking properly. Just found the real test under net.tests.test_fido

nabobalis · 2018-03-13T12:13:18Z

sunpy/net/dataretriever/client.py

+        elif isinstance(path, pathlib.Path):
+            path = str(path.absolute())
+        else:
+            err = 'path should be either pathlib.Path object or str object. '\


Not sure I like using the word object here.

What is a 'word' object?

nabobalis · 2018-03-13T12:41:58Z

sunpy/net/tests/test_fido.py

@@ -97,6 +98,14 @@ def test_save_path():
            assert target_dir in f
            assert "eve{}0".format(os.path.sep) in f

+    with tempfile.TemporaryDirectory() as target_dir:
+        qr = Fido.search(a.Instrument('EVE'), a.Time("2016/10/01", "2016/10/02"), a.Level(0))
+        path = pathlib.Path(os.path.join(target_dir, "{instrument}"+os.path.sep+"{level}"))


Do pathlib.Path need to use os.path.join?
I had thought that pathlib.Path was meant to handle that.

Missed it while copy-pasting!

nabobalis · 2018-03-13T12:42:25Z

sunpy/net/tests/test_fido.py

@@ -97,6 +98,14 @@ def test_save_path():
            assert target_dir in f
            assert "eve{}0".format(os.path.sep) in f

+    with tempfile.TemporaryDirectory() as target_dir:


I do wonder if we should split it into two tests.

We are downloading the same files. So I figured downloading the files into a different directory would be the best option to test for flaws.

Well in that case, do we need to redo the Fido.search part? Could we not just fetch it twice one after the other?

Should I split it into two tests? @nabobalis

Could we not just fetch the files twice with two different paths?

Now the searching is only done once.

Can you add a comment somewhere to just state what the difference in the two parts of the test are.

It is in a different test now! ^_^

Add changelog Add keyword description in docstring Change error message yo requested format Remove recurrent os.path.join

nabobalis · 2018-03-13T13:34:51Z

sunpy/net/tests/test_fido.py

+    import pathlib
+    HAS_PATHLIB = True
+except ImportError:
+    HAS_PATHLIB = False


Would we not need this for the main file as well?

instead of doing this we should use pytest.importorskip("pathlib") in the test

nabobalis · 2018-03-14T12:52:31Z

Looks my logic doesn't work:

TypeError: path should be either 'pathlib.Path' or 'str'. Got '<class 'NoneType'>'.


Exception occurred:
  File "/home/circleci/project/build/lib.linux-x86_64-3.6/sunpy/net/dataretriever/client.py", line 360, in fetch
    raise TypeError(err)

nabobalis · 2018-03-14T12:54:21Z

sunpy/net/dataretriever/client.py

        Returns
        -------
        Results Object
        """
+        # Check for type of path
+        if path is not None and HAS_PATHLIB and isinstance(path, pathlib.Path):


You want if path is not None to encompass the other if/elif

Oops! Sorry!

nabobalis · 2018-03-14T13:06:31Z

sunpy/net/dataretriever/client.py

@@ -8,6 +8,15 @@
 from abc import ABCMeta
 from collections import OrderedDict, namedtuple
 from functools import partial
+import six


You probably want this to be import sunpy.extern.six.

dpshelio · 2018-03-15T12:41:13Z

sunpy/net/tests/test_fido.py

        files = Fido.fetch(qr, path=os.path.join(target_dir, "{instrument}"+os.path.sep+"{level}"))
        for f in files:
            assert target_dir in f
            assert "eve{}0".format(os.path.sep) in f

+    # Test when path is pathlib.Path


Shouldn't this go in its own test? It would be easier to manage and control when one or the other fails.

dpshelio · 2018-03-15T12:43:30Z

sunpy/net/tests/test_fido.py

    with tempfile.TemporaryDirectory() as target_dir:
-        qr = Fido.search(a.Instrument('EVE'), a.Time("2016/10/01", "2016/10/02"), a.Level(0))
        files = Fido.fetch(qr, path=os.path.join(target_dir, "{instrument}"+os.path.sep+"{level}"))


os.path.sep is not needed there... unless that's what you want to test. os.path.join does it for you.

files = Fido.fetch(qr, path=os.path.join(target_dir, "{instrument}", "{level}"))

dpshelio · 2018-03-15T12:44:54Z

sunpy/net/tests/test_fido.py

+    # Test when path is pathlib.Path
+    if HAS_PATHLIB:
+        with tempfile.TemporaryDirectory() as target_dir:
+            path = pathlib.Path(target_dir, "{instrument}"+os.path.sep+"{level}")


You don't need it here either, pathlib will add the sep for each argument:

path = pathlib.Path(target_dir, "{instrument}", "{level}")

nabobalis

Looks good to me.

nabobalis · 2018-03-16T08:55:39Z

This also now fixes the two known test failures on the online tests.

vn-ki · 2018-03-18T15:53:21Z

This can be merged now, right?

nabobalis · 2018-03-18T15:55:36Z

I have requested a review and I am waiting on that before I merge this.

But as far as I am concerned, yes it can be merged in.

Cadair · 2018-03-21T17:59:54Z

sunpy/net/dataretriever/client.py

@@ -9,6 +9,14 @@
 from collections import OrderedDict, namedtuple
 from functools import partial

+# Remove in 1.0


can you modify this comment to start with python 3: as I have been using that in a few places for post-0.9 grepping.

Cadair · 2018-03-21T18:00:40Z

sunpy/net/tests/test_fido.py

+    import pathlib
+    HAS_PATHLIB = True
+except ImportError:
+    HAS_PATHLIB = False


instead of doing this we should use pytest.importorskip("pathlib") in the test

nabobalis · 2018-03-21T19:22:20Z

sunpy/net/tests/test_fido.py

+
+@pytest.mark.remote_data
+def test_save_path_pathlib():
+    pathlib = pytest.importorskip('pathlib')


Is this not a decorator?

no, decorators are run at import time.

nabobalis · 2018-03-21T19:22:41Z

We updated the Changelog, you will need to fix the conflict now.

vn-ki added 3 commits March 13, 2018 16:59

Fido.fetch now accept pathlib.Path objects

58edb1f

Add tests for path attribute of Fido.fetch

606302b

Fix small error in dataretriver.client

60eb247

nabobalis reviewed Mar 13, 2018

View reviewed changes

nabobalis added net Affects the net submodule No Changelog Entry Needed labels Mar 13, 2018

nabobalis reviewed Mar 13, 2018

View reviewed changes

Add test in the proper file

ff6811a

nabobalis removed the No Changelog Entry Needed label Mar 13, 2018

nabobalis reviewed Mar 13, 2018

View reviewed changes

Make the requested changes

53be326

Add changelog Add keyword description in docstring Change error message yo requested format Remove recurrent os.path.join

vn-ki force-pushed the accept-pathlibPath-objects branch from b31ecb9 to 53be326 Compare March 13, 2018 12:58

nabobalis reviewed Mar 13, 2018

View reviewed changes

Fix test failure for py27

d767404

vn-ki force-pushed the accept-pathlibPath-objects branch from eab6efe to d767404 Compare March 13, 2018 13:53

nabobalis reviewed Mar 14, 2018

View reviewed changes

vn-ki force-pushed the accept-pathlibPath-objects branch from 9f0808f to d66b2e3 Compare March 14, 2018 12:58

nabobalis reviewed Mar 14, 2018

View reviewed changes

nabobalis requested a review from Cadair March 14, 2018 13:10

Remove if: pass from client.py

3b226ce

vn-ki force-pushed the accept-pathlibPath-objects branch from d66b2e3 to 3b226ce Compare March 14, 2018 13:17

dpshelio requested changes Mar 15, 2018

View reviewed changes

vn-ki added 2 commits March 15, 2018 22:39

Make pathlib test a different test

9995f3b

Fix tests in database.tests.test_database

82365f8

nabobalis approved these changes Mar 16, 2018

View reviewed changes

nabobalis added this to the 0.9 milestone Mar 16, 2018

dpshelio approved these changes Mar 16, 2018

View reviewed changes

Cadair requested changes Mar 21, 2018

View reviewed changes

Fix comment and use importorskip

2ff8464

nabobalis reviewed Mar 21, 2018

View reviewed changes

Merge branch 'master' into accept-pathlibPath-objects

9b0f039

Cadair approved these changes Mar 22, 2018

View reviewed changes

Cadair merged commit 4cfae7d into sunpy:master Mar 22, 2018

vn-ki deleted the accept-pathlibPath-objects branch May 11, 2018 18:02

Fido.fetch now accepts pathlib.Path objects #2559

Fido.fetch now accepts pathlib.Path objects #2559

Conversation

vn-ki commented Mar 13, 2018

pep8speaks commented Mar 13, 2018 • edited

Comment last updated on March 21, 2018 at 20:18 Hours UTC

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

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

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

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

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

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

nabobalis commented Mar 14, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nabobalis left a comment

Choose a reason for hiding this comment

nabobalis commented Mar 16, 2018

vn-ki commented Mar 18, 2018

nabobalis commented Mar 18, 2018

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

nabobalis commented Mar 21, 2018

pep8speaks commented Mar 13, 2018 •

edited