diff --git a/nbs/api/00_core.ipynb b/nbs/api/00_core.ipynb index add8b5d5..7d3838a7 100644 --- a/nbs/api/00_core.ipynb +++ b/nbs/api/00_core.ipynb @@ -1327,7 +1327,9 @@ } ], "source": [ - "qp('/foo', **vals)" + "res = qp('/foo', **vals)\n", + "assert res == '/foo?a=5&b=&c=1&c=2&d=bar&e='\n", + "res" ] }, { @@ -1356,7 +1358,8 @@ } ], "source": [ - "qp('/foo/{a}',**vals)" + "res = qp('/foo/{a}',**vals)\n", + "assert res == '/foo/5?b=&c=1&c=2&d=bar&e='" ] }, { @@ -1385,7 +1388,9 @@ } ], "source": [ - "qp('/foo/{a}/{d}', **vals)" + "res = qp('/foo/{a}/{d}', **vals)\n", + "assert res == '/foo/5/bar?b=&c=1&c=2&e='\n", + "res" ] }, { @@ -3359,83 +3364,6 @@ " return send" ] }, - { - "cell_type": "code", - "execution_count": null, - "id": "bd362813", - "metadata": {}, - "outputs": [], - "source": [ - "%%ai\n", - "\n", - "Is there a better way to implement the `qp` function, particularly the `for k,v in kw.items()` loop? Should this logic happen somewhere else so it's not all string replacement stuff?" - ] - }, - { - "cell_type": "markdown", - "id": "3e6b7e02", - "metadata": {}, - "source": [ - "Looking at the `qp` function's implementation, there are a few potential improvements we could make:\n", - "\n", - "1. The string replacement approach is error-prone since it doesn't handle nested URL parameters properly and could accidentally replace substrings that happen to match parameter names.\n", - "\n", - "2. We could use Starlette's built-in URL path building functionality instead of doing string manipulation ourselves.\n", - "\n", - "Here's a suggested improved implementation:\n", - "\n", - "```python\n", - "def qp(p:str, **kw) -> str:\n", - " \"Add query parameters to path p\"\n", - " # Handle empty/false values\n", - " kw = {k:('' if v in (False,None) else v) for k,v in kw.items()}\n", - " \n", - " # Split path into base and any existing query params\n", - " base,_,existing = p.partition('?')\n", - " \n", - " # Use Starlette's URL builder for path params\n", - " if '{' in base:\n", - " path_params = {}\n", - " query_params = {}\n", - " for k,v in kw.items():\n", - " if f'{{{k}}}' in base: path_params[k] = v\n", - " else: query_params[k] = v\n", - " base = base.format(**path_params)\n", - " kw = query_params\n", - " \n", - " # Combine query params\n", - " query = urlencode(kw, doseq=True)\n", - " if existing: query = f'{existing}&{query}'\n", - " \n", - " return f'{base}{\"?\" if query else \"\"}{query}'\n", - "```\n", - "\n", - "This improved version:\n", - "\n", - "1. Properly handles path parameters using Python's string formatting\n", - "2. Preserves any existing query parameters in the path\n", - "3. Clearly separates path parameter substitution from query parameter addition\n", - "4. Still maintains the empty value handling for False/None values\n", - "5. Uses `doseq=True` for proper list parameter handling\n", - "\n", - "You could test it with:\n", - "\n", - "```python\n", - "assert qp('/foo/{a}', a=5, b=None) == '/foo/5?b='\n", - "assert qp('/foo/{a}/{d}', **vals) == '/foo/5/bar?b=&c=1&c=2&e='\n", - "assert qp('/foo?x=1', y=2) == '/foo?x=1&y=2'\n", - "```\n", - "\n", - "This implementation is more robust while still being relatively simple. The main improvement is properly handling path parameters using Python's string formatting rather than trying to detect and replace them manually.\n", - "\n", - "The logic belongs in this function rather than elsewhere because:\n", - "1. It's a URL utility function specifically for combining paths and parameters\n", - "2. It needs to handle both path and query parameters in a coordinated way\n", - "3. The implementation is self-contained and doesn't require external state\n", - "\n", - "Would you like me to explain any part of this in more detail?" - ] - }, { "cell_type": "markdown", "id": "474e14b4",