Repository: dr-leo/pandaSDMX (python)

Issue:
Convert Request.url() to a top-level convenience method
As raised by @dr-leo via e-mail:

> On Request.url: I'd prefer a confenience function rather than a classmethod. It is wierd that a single class provides two ways to do the same thing (alongside with url kwarg of .get()). A convenience function 'get_url' or something would instantiate a Request obj as does the classmethod, but pass kwargs, in particular use_cache to its get() meth. Thus, the cache may be used even from the convenience function. But this is a minor quirk. I'd be fine with removing the classmethod in the first place so users must instantiate themselves.

Pull request:
Convert Request.url() to a top-level convenience method
Closes #85.

Patch:
diff --git a/pandasdmx/__init__.py b/pandasdmx/__init__.py
index c93b0f0..677c546 100644
--- a/pandasdmx/__init__.py
+++ b/pandasdmx/__init__.py
@@ -1,4 +1,4 @@
-from pandasdmx.api import Request, open_file
+from pandasdmx.api import Request, open_file, read_url
 from pandasdmx.source import add_source, list_sources
 from pandasdmx.writer import write as to_pandas
 import logging
@@ -8,6 +8,7 @@ __all__ = [
     'add_source',
     'list_sources',
     'open_file',
+    'read_url',
     'to_pandas',
     ]
 
diff --git a/pandasdmx/api.py b/pandasdmx/api.py
index 48e22db..60e179b 100644
--- a/pandasdmx/api.py
+++ b/pandasdmx/api.py
@@ -46,11 +46,6 @@ class Request:
     #: :class:`pandasdmx.source.Source` for requests sent from the instance.
     source = None
 
-    @classmethod
-    def url(cls, url, **kwargs):
-        """Request a URL directly."""
-        return Request().get(url=url, **kwargs)
-
     def __init__(self, source=None, log_level=None, **session_opts):
         """Constructor."""
         try:
@@ -130,18 +125,11 @@ class Request:
 
     def _request_from_args(self, **kwargs):
         """Validate arguments and prepare pieces for a request."""
-        # Allow sources to modify request args
-        # TODO this should occur after most processing, defaults, checking etc.
-        #      are performed, so that core code does most of the work.
-        if self.source:
-            self.source.modify_request_args(kwargs)
-
         parameters = kwargs.pop('params', {})
         headers = kwargs.pop('headers', {})
 
         # Base URL
-        direct_url = kwargs.pop('url', None)
-        url_parts = [direct_url] if direct_url else [self.source.url]
+        url_parts = [self.source.url]
 
         # Resource arguments
         resource = kwargs.pop('resource', None)
@@ -172,13 +160,12 @@ class Request:
                 resource_id = resource.id
 
         force = kwargs.pop('force', False)
-        if not (force or direct_url or self.source.supports[resource_type]):
+        if not (force or self.source.supports[resource_type]):
             raise NotImplementedError(f'{self.source.id} does not support the'
                                       f'{resource_type!r} API endpoint; '
                                       'override using force=True')
 
-        if not direct_url:
-            url_parts.append(resource_type.name)
+        url_parts.append(resource_type.name)
 
         # Agency ID to use in the URL
         agency = kwargs.pop('agency', None)
@@ -190,12 +177,10 @@ class Request:
         else:
             agency_id = agency if agency else getattr(self.source, 'id', None)
 
-        if not direct_url:
-            url_parts.extend([agency_id, resource_id])
+        url_parts.extend([agency_id, resource_id])
 
         version = kwargs.pop('version', None)
-        if not version and (resource_type != Resource.data
-                            and not direct_url):
+        if not version and resource_type != Resource.data:
             url_parts.append('latest')
 
         key = kwargs.pop('key', None)
@@ -220,8 +205,18 @@ class Request:
         if not headers and self.source and resource_type:
             headers = self.source.headers.get(resource_type.name, {})
 
-        assert len(kwargs) == 0, ValueError('unrecognized arguments: %r' %
-                                            kwargs)
+        if len(kwargs):
+            raise ValueError(f'unrecognized arguments: {kwargs!r}')
+
+        return requests.Request('get', url, params=parameters,
+                                headers=headers)
+
+    def _request_from_url(self, url, **kwargs):
+        parameters = kwargs.pop('params', {})
+        headers = kwargs.pop('headers', {})
+
+        if len(kwargs):
+            raise ValueError(f'unrecognized arguments: {kwargs!r}')
 
         return requests.Request('get', url, params=parameters,
                                 headers=headers)
@@ -256,7 +251,7 @@ class Request:
             Object to get.
         url : str
             Full URL to get directly. If given, other arguments are ignored.
-            See also :meth:`url`.
+            See also :meth:`read_url`.
         agency : str
             ID of the agency providing the data or metadata.
             Used for URL construction only. It tells the SDMX web service
@@ -297,10 +292,20 @@ class Request:
         :class:`pandasdmx.message.Message`
             The requested SDMX message.
         """
-        req = self._request_from_args(
-            resource_type=resource_type,
-            resource_id=resource_id,
-            **kwargs)
+        # Allow sources to modify request args
+        # TODO this should occur after most processing, defaults, checking etc.
+        #      are performed, so that core code does most of the work.
+        if self.source:
+            self.source.modify_request_args(kwargs)
+
+        if 'url' in kwargs:
+            req = self._request_from_url(**kwargs)
+        else:
+            req = self._request_from_args(
+                resource_type=resource_type,
+                resource_id=resource_id,
+                **kwargs)
+
         req = self.session.prepare_request(req)
 
         # Now get the SDMX message via HTTP
@@ -446,11 +451,13 @@ def open_file(filename_or_obj, format=None):
             reader = readers[format]
             obj = open(str(filename_or_obj))
         else:
-            raise RuntimeError(('unable to identify SDMX file format from '
-                                'name "{}"; use format="..."')
-                               .format(filename_or_obj.name))
+            msg = ("cannot identify SDMX message format from file name "
+                   f"'{filename_or_obj.name}'; use  format='...'")
+            raise RuntimeError(msg)
     except AttributeError:
         # File is already open
+
+        # Read a line and then return the cursor to the initial position
         pos = filename_or_obj.tell()
         first_line = filename_or_obj.readline().strip()
         filename_or_obj.seek(pos)
@@ -460,8 +467,14 @@ def open_file(filename_or_obj, format=None):
         elif first_line.startswith('<'):
             reader = readers['XML']
         else:
-            raise ValueError(first_line)
+            msg = f"cannot infer SDMX message format from '{first_line[:5]}..'"
+            raise RuntimeError(msg)
 
         obj = filename_or_obj
 
     return reader().read_message(obj)
+
+
+def read_url(url, **kwargs):
+    """Request a URL directly."""
+    return Request().get(url=url, **kwargs)


Test patch:
diff --git a/tests/__init__.py b/tests/__init__.py
index c0ce3d7..c4c1cf6 100644
--- a/tests/__init__.py
+++ b/tests/__init__.py
@@ -54,12 +54,12 @@ def test_files(format=None, kind=None):
 
 
 @contextmanager
-def specimen(pattern=''):
+def specimen(pattern='', opened=True):
     """Open the test specimen file with *match* in the name."""
     for path, f, k in _test_files:
         if path.match('*' + pattern + '*'):
             # str() here is for Python 3.5 compatibility
-            yield open(str(path))
+            yield open(str(path)) if opened else path
             break
 
 
diff --git a/tests/test_api.py b/tests/test_api.py
index 39e8615..8dd7a3e 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -1,14 +1,38 @@
+from io import StringIO
 import logging
 
 import pandas as pd
 import pandasdmx as sdmx
 import pytest
 
+from . import specimen
+
+
+def test_open_file(tmp_path):
+    # Copy the file to a temporary file with an urecognizable suffix
+    target = tmp_path / 'foo.badsuffix'
+    with specimen('exr-flat.json', opened=False) as original:
+        target.open('w').write(original.read_text())
 
-def test_open_file():
     # Can't infer message type for unknown file extension
-    with pytest.raises(RuntimeError):
-        sdmx.open_file('foo.badsuffix')
+    exc = ("cannot identify SDMX message format from file name 'foo.badsuffix'"
+           "; use  format='...'")
+    with pytest.raises(RuntimeError, match=exc):
+        sdmx.open_file(target)
+
+    # Using the format= kwarg suppresses the error
+    sdmx.open_file(target, format='JSON')
+
+    # Format can be inferred from an already-open file without extension
+    with specimen('exr-flat.json') as f:
+        sdmx.open_file(f)
+
+    # Exception raised when the file contents don't allow to guess the format
+    bad_file = StringIO('#! neither XML nor JSON')
+    exc = "cannot infer SDMX message format from '#! ne..'"
+    with pytest.raises(RuntimeError, match=exc):
+        sdmx.open_file(bad_file)
+
 
 
 def test_request():
@@ -26,6 +50,19 @@ def test_request():
     assert r.timeout == 300
 
 
+def test_request_get_exceptions():
+    """Tests of Request.get() that don't require remote data."""
+    req = sdmx.Request('ESTAT')
+
+    # Exception is raised on unrecognized arguments
+    exc = "unrecognized arguments: {'foo': 'bar'}"
+    with pytest.raises(ValueError, match=exc):
+        req.get('datastructure', foo='bar')
+
+    with pytest.raises(ValueError, match=exc):
+        sdmx.read_url('https://example.com', foo='bar')
+
+
 @pytest.mark.remote_data
 def test_request_get():
     req = sdmx.Request('ESTAT')
@@ -58,10 +95,10 @@ def test_request_make_series_key():
 
 
 @pytest.mark.remote_data
-def test_request_url():
+def test_read_url():
     # URL can be queried without instantiating Request
-    sdmx.Request.url('http://sdw-wsrest.ecb.int/service/datastructure/ECB/'
-                     'ECB_EXR1/latest?references=all')
+    sdmx.read_url('http://sdw-wsrest.ecb.int/service/datastructure/ECB/'
+                  'ECB_EXR1/latest?references=all')
 
 
 @pytest.mark.remote_data
diff --git a/tests/test_model.py b/tests/test_model.py
index 781201e..88d3763 100644
--- a/tests/test_model.py
+++ b/tests/test_model.py
@@ -38,7 +38,7 @@ def test_dataset():
     # Enumeration values can be used to initialize
     from pandasdmx.model import ActionType
     print(ActionType)
-    ds = DataSet(action=ActionType['information'])
+    DataSet(action=ActionType['information'])
 
 
 def test_datastructuredefinition():