Repository: ithaka/apiron

Issue:
Replace path_kwargs with introspected kwargs
**Is your feature request related to a problem? Please describe.**
It would feel more Pythonic and ease development a bit if `path_kwargs` were replaced with introspected keyword arguments.

**Describe the solution you'd like**

Given a service:

```python
class PokeAPI(Service):
    domain = 'https://pokeapi.co'
    pokemon = JsonEndpoint(path='/api/v2/pokemon/{pokemon}')
```

Before:

```python
ditto = PokeAPI.pokemon(path_kwargs={'pokemon': 'ditto'})
```

After:

```python
ditto = PokeAPI.pokemon(pokemon='ditto')
```

**Describe alternatives you've considered**
You could almost get this using a `dict` constructor, but you still need the `path_kwargs=` in some/most cases, which is longer than the "before" above:

```python
ditto = PokeAPI.pokemon(path_kwargs=dict(pokemon='ditto'))
```

**Additional context**
This can be achieved by checking if a `kwarg` passed to `ServiceCaller.call` is one of the kwargs expected in the `path` of the `Endpoint` being called. Conflicts could arise if a path kwarg is the same name as any of the current/future explicit kwargs `ServiceCaller.call` expects. When a kwarg that is unexpected by either of `ServiceCaller.call` or the `Endpoint` is passed, things should probably fail loudly and clearly, i.e. a `ValueError`.


Discussion:

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 8772bb3..4e5503e 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+### Added
+- Ability to use regular keyword arguments when calling an endpoint (instead of `path_kwargs`)
+
 ## [2.4.0] - 2019-04-09
 ### Changed
 - Simplify imports so that all commonly-used classes can be imported with `from apiron import <class>`
diff --git a/README.md b/README.md
index 7cc25aa..6af0476 100644
--- a/README.md
+++ b/README.md
@@ -40,13 +40,11 @@ class GitHub(Service):
 Once your service definition is in place, you can interact with its endpoints:
 
 ```python
-response = GitHub.user(
-    path_kwargs={'username': 'defunkt'},
-)  # {"name": "Chris Wanstrath", ...}
+response = GitHub.user(username='defunkt')
+# {"name": "Chris Wanstrath", ...}
 
-response = GitHub.repo(
-    path_kwargs={'org': 'github', 'repo': 'hub'},
-)  # {"description": "hub helps you win at git.", ...}
+response = GitHub.repo(org='github', repo='hub')
+# {"description": "hub helps you win at git.", ...}
 ```
 
 To learn more about building clients, head over to [the docs](https://apiron.readthedocs.io).
diff --git a/apiron/client.py b/apiron/client.py
index 8070302..333e30b 100644
--- a/apiron/client.py
+++ b/apiron/client.py
@@ -1,6 +1,7 @@
 import collections
 import logging
 import random
+import warnings
 from urllib import parse
 
 import requests
@@ -108,10 +109,20 @@ class ServiceCaller:
         headers=None,
         cookies=None,
         auth=None,
+        **kwargs
     ):
         host = cls.choose_host(service=service)
 
+        if path_kwargs:
+            warnings.warn(
+                'path_kwargs is no longer necessary and will be removed in a future version of apiron. '
+                'You can call endpoints using plain keyword arguments instead!',
+                RuntimeWarning,
+                stacklevel=4
+            )
+
         path_kwargs = path_kwargs or {}
+        path_kwargs.update(**kwargs)
         path = endpoint.get_formatted_path(**path_kwargs)
 
         merged_params = endpoint.get_merged_params(params)
@@ -148,6 +159,7 @@ class ServiceCaller:
         retry_spec=DEFAULT_RETRY,
         timeout_spec=DEFAULT_TIMEOUT,
         logger=None,
+        **kwargs
     ):
         """
         :param Service service:
@@ -240,6 +252,7 @@ class ServiceCaller:
             headers=headers,
             cookies=cookies,
             auth=auth,
+            **kwargs
         )
 
         logger.info('{method} {url}'.format(
diff --git a/apiron/endpoint/endpoint.py b/apiron/endpoint/endpoint.py
index 9dc84be..26c4df6 100644
--- a/apiron/endpoint/endpoint.py
+++ b/apiron/endpoint/endpoint.py
@@ -1,4 +1,5 @@
 import logging
+import string
 import warnings
 
 from apiron.exceptions import UnfulfilledParameterException
@@ -85,8 +86,38 @@ class Endpoint:
         :rtype:
             str
         """
+        self._validate_path_placeholders(self.path_placeholders, kwargs)
+
         return self.path.format(**kwargs)
 
+    @property
+    def path_placeholders(self):
+        """
+        The formattable placeholders from this endpoint's path, in the order they appear.
+
+        Example:
+
+            >>> endpoint = Endpoint(path='/api/{foo}/{bar}')
+            >>> endpoint.path_placeholders
+            ['foo', 'bar']
+        """
+
+        parser = string.Formatter()
+        return [
+            placeholder_name
+            for _, placeholder_name, _, _ in parser.parse(self.path)
+            if placeholder_name
+        ]
+
+    def _validate_path_placeholders(self, placeholder_names, path_kwargs):
+        if any(path_kwarg not in placeholder_names for path_kwarg in path_kwargs):
+            warnings.warn(
+                'An unknown path kwarg was supplied to {}. '
+                'kwargs supplied: {}'.format(self, path_kwargs),
+                RuntimeWarning,
+                stacklevel=6,
+            )
+
     def get_merged_params(self, supplied_params=None):
         """
         Merge this endpoint's default parameters with the supplied parameters
@@ -116,7 +147,7 @@ class Endpoint:
                     empty_params=empty_params,
                 ),
                 RuntimeWarning,
-                stacklevel=4,
+                stacklevel=5,
             )
 
         unfulfilled_params = {
diff --git a/dev-requirements.txt b/dev-requirements.txt
index 819389c..b119b39 100644
--- a/dev-requirements.txt
+++ b/dev-requirements.txt
@@ -1,5 +1,6 @@
 -r requirements.txt
 
+pytest==4.2.0
 pytest-cov==2.6.1
 sphinx==1.7.6
 sphinx-autobuild==0.7.1
diff --git a/docs/advanced-usage.rst b/docs/advanced-usage.rst
index d56ff28..ac7d2c7 100644
--- a/docs/advanced-usage.rst
+++ b/docs/advanced-usage.rst
@@ -46,11 +46,11 @@ Using all the features
     HttpBin.poster(data={'foo': 'bar'})
 
     # A GET call with parameters formatted into the path
-    HttpBin.anything(path_kwargs={'anything': 42})
+    HttpBin.anything(anything=42)
 
     # A GET call with a 500 response, raises RetryError since we successfully tried but got a bad response
     try:
-        HttpBin.status(path_kwargs={'status_code': 500})
+        HttpBin.status(status_code=500)
     except requests.exceptions.RetryError:
         pass
 
@@ -66,7 +66,7 @@ Using all the features
     )
 
     # A streaming response
-    response = HttpBin.streamer(path_kwargs={'num_lines': 20})
+    response = HttpBin.streamer(num_lines=20)
     for chunk in response:
         print(chunk)
 
diff --git a/docs/getting-started.rst b/docs/getting-started.rst
index 2b0c9f1..db185d2 100644
--- a/docs/getting-started.rst
+++ b/docs/getting-started.rst
@@ -38,13 +38,11 @@ using the :class:`ServiceCaller <apiron.client.ServiceCaller>`:
 
 .. code-block:: python
 
-    response = GitHub.user(
-        path_kwargs={'username': 'defunkt'},
-    )  # {"name": "Chris Wanstrath", ...}
+    response = GitHub.user(username='defunkt')
+    # {"name": "Chris Wanstrath", ...}
 
-    response = GitHub.repo(
-        path_kwargs={'org': 'github', 'repo': 'hub'},
-    )  # {"description": "hub helps you win at git.", ...}
+    response = GitHub.repo(org='github', repo='hub')
+    # {"description": "hub helps you win at git.", ...}
 
 
 **********


Test patch:
diff --git a/tests/test_client.py b/tests/test_client.py
index b294f28..3a8741b 100644
--- a/tests/test_client.py
+++ b/tests/test_client.py
@@ -4,6 +4,7 @@ import pytest
 
 from apiron import ServiceCaller, NoHostsAvailableException
 
+
 class TestClient:
     @mock.patch('requests.sessions.Session', autospec=True)
     def test_get_adapted_session(self, mock_session):
@@ -41,7 +42,6 @@ class TestClient:
         endpoint.default_params = {}
         endpoint.required_params = set()
 
-        path_kwargs = {'foo': 'bar'}
         params = {'baz': 'qux'}
         endpoint.get_merged_params.return_value = params
         data = 'I am a data'
@@ -59,12 +59,12 @@ class TestClient:
                 session,
                 service,
                 endpoint,
-                path_kwargs=path_kwargs,
                 params=params,
                 data=data,
                 headers=headers,
                 cookies=cookies,
                 auth=auth,
+                foo='bar',
             )
 
             mock_request_constructor.assert_called_once_with(
diff --git a/tests/test_endpoint.py b/tests/test_endpoint.py
index e8f7890..f37d913 100644
--- a/tests/test_endpoint.py
+++ b/tests/test_endpoint.py
@@ -48,6 +48,14 @@ class TestEndpoint:
         foo = apiron.Endpoint(path='/foo/bar/')
         assert '/foo/bar/' == str(foo)
 
+    def test_path_placeholders_when_none_present(self):
+        foo = apiron.Endpoint()
+        assert [] == foo.path_placeholders
+
+    def test_path_placeholders_when_present(self):
+        foo = apiron.Endpoint(path='/foo/{one}/{two}')
+        assert ['one', 'two'] == foo.path_placeholders
+
     def test_format_path_with_correct_kwargs(self):
         foo = apiron.Endpoint(path='/{one}/{two}/')
         path_kwargs = {'one': 'foo', 'two': 'bar'}
@@ -57,17 +65,24 @@ class TestEndpoint:
         foo = apiron.Endpoint(path='/{one}/{two}/')
         path_kwargs = {'foo': 'bar'}
         with pytest.raises(KeyError):
-            foo.get_formatted_path(**path_kwargs)
+            with warnings.catch_warnings(record=True) as warning_records:
+                warnings.simplefilter('always')
+                foo.get_formatted_path(**path_kwargs)
+            assert 1 == len(warning_records)
+            assert issubclass(warning_records[-1].category, RuntimeWarning)
 
     def test_format_path_with_extra_kwargs(self):
         foo = apiron.Endpoint(path='/{one}/{two}/')
         path_kwargs = {'one': 'foo', 'two': 'bar', 'three': 'not used'}
-        assert '/foo/bar/' == foo.get_formatted_path(**path_kwargs)
+        with warnings.catch_warnings(record=True) as warning_records:
+            assert '/foo/bar/' == foo.get_formatted_path(**path_kwargs)
+            assert 1 == len(warning_records)
+            assert issubclass(warning_records[-1].category, RuntimeWarning)
 
     def test_query_parameter_in_path_generates_warning(self):
         with warnings.catch_warnings(record=True) as warning_records:
             warnings.simplefilter('always')
-            foo = apiron.Endpoint(path='/?foo=bar')
+            _ = apiron.Endpoint(path='/?foo=bar')
             assert 1 == len(warning_records)
             assert issubclass(warning_records[-1].category, UserWarning)
 
@@ -79,7 +94,7 @@ class TestEndpoint:
         foo = apiron.JsonEndpoint(default_params={'foo': 'bar'}, required_params={'baz'})
 
         with pytest.raises(apiron.UnfulfilledParameterException):
-            foo.get_merged_params(None)
+            foo.get_merged_params()
 
     def test_get_merged_params_with_empty_param(self):
         foo = apiron.JsonEndpoint(default_params={'foo': 'bar'}, required_params={'baz'})
@@ -92,7 +107,16 @@ class TestEndpoint:
 
     def test_get_merged_params_with_required_and_default_param(self):
         foo = apiron.JsonEndpoint(default_params={'foo': 'bar'}, required_params={'foo'})
-        assert {'foo': 'bar'} == foo.get_merged_params(None)
+        assert {'foo': 'bar'} == foo.get_merged_params()
+
+    @mock.patch('apiron.client.requests.Session.send')
+    def test_using_path_kwargs_produces_warning(self, mock_send, service):
+        service.foo = apiron.Endpoint(path='/foo/{one}')
+        with warnings.catch_warnings(record=True) as warning_records:
+            warnings.simplefilter('always')
+            _ = service.foo(path_kwargs={'one': 'bar'})
+            assert 1 == len(warning_records)
+            assert issubclass(warning_records[-1].category, RuntimeWarning)
 
 
 class TestJsonEndpoint:
@@ -127,7 +151,6 @@ class TestStreamingEndpoint:
 
 
 class TestStubEndpoint:
-
     def test_stub_response(self):
         """
         Test initializing a stub endpoint with a stub response
@@ -189,7 +212,7 @@ class TestStubEndpoint:
             expected_response={'stub response': 'for param_key=param_value'},
         )
 
-    def test_call_without_service_raises_exception(self, service):
+    def test_call_without_service_raises_exception(self):
         stub_endpoint = apiron.StubEndpoint(stub_response='foo')
         with pytest.raises(TypeError):
             stub_endpoint()