Skip to content

Commit

Permalink
clean
Browse files Browse the repository at this point in the history
  • Loading branch information
Isaac-Flath committed Jan 19, 2025
1 parent 254d5e6 commit 7b793f4
Showing 1 changed file with 8 additions and 80 deletions.
88 changes: 8 additions & 80 deletions nbs/api/00_core.ipynb
Original file line number Diff line number Diff line change
Expand Up @@ -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"
]
},
{
Expand Down Expand Up @@ -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='"
]
},
{
Expand Down Expand Up @@ -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"
]
},
{
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 7b793f4

Please sign in to comment.