Skip to content

Commit

Permalink
Handling for query or url params in qp/to
Browse files Browse the repository at this point in the history
  • Loading branch information
@Isaac-Flath
Isaac-Flath committed Jan 19, 2025
1 parent efd1916 commit 254d5e6
Show file tree
Hide file tree
Showing 2 changed files with 197 additions and 14 deletions.
9 changes: 8 additions & 1 deletion fasthtml/core.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,14 @@ async def _f(req, exc):
# %% ../nbs/api/00_core.ipynb
def qp(p:str, **kw) -> str:
"Add query parameters to path p"
kw = {k:('' if v in (False,None) else v) for k,v in kw.items()}
kw = {k:('' if v in (False,None) else v) for k,v in kw.items()}
_mods = []
for k,v in kw.items():
_substr=f"{{{k}}}"
if _substr in p:
_mods.append(k)
p = p.replace(_substr,str(v))
kw = {k:v for k,v in kw.items() if k not in _mods}
return p + ('?' + urlencode(kw,doseq=True) if kw else '')

# %% ../nbs/api/00_core.ipynb
Expand Down
202 changes: 189 additions & 13 deletions nbs/api/00_core.ipynb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@
{
"data": {
"text/plain": [
"datetime.datetime(2025, 1, 12, 14, 0)"
"datetime.datetime(2025, 1, 19, 14, 0)"
]
},
"execution_count": null,
Expand Down Expand Up @@ -1230,7 +1230,7 @@
{
"data": {
"text/plain": [
"'5a5e5544-5ee8-46f2-836e-924976ce8b58'"
"'7293b76e-03e7-4cba-9a48-346d25aa5f32'"
]
},
"execution_count": null,
Expand Down Expand Up @@ -1280,10 +1280,35 @@
"#| export\n",
"def qp(p:str, **kw) -> str:\n",
" \"Add query parameters to path p\"\n",
" kw = {k:('' if v in (False,None) else v) for k,v in kw.items()}\n",
" kw = {k:('' if v in (False,None) else v) for k,v in kw.items()} \n",
" _mods = []\n",
" for k,v in kw.items(): \n",
" _substr=f\"{{{k}}}\"\n",
" if _substr in p:\n",
" _mods.append(k)\n",
" p = p.replace(_substr,str(v))\n",
" kw = {k:v for k,v in kw.items() if k not in _mods}\n",
" return p + ('?' + urlencode(kw,doseq=True) if kw else '')"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "ebd2b367",
"metadata": {},
"outputs": [],
"source": [
"vals = {'a':5, 'b':False, 'c':[1,2], 'd':'bar', 'e':None}"
]
},
{
"cell_type": "markdown",
"id": "e51e2dc3",
"metadata": {},
"source": [
"`qp` adds query parameters to route path strings"
]
},
{
"cell_type": "code",
"execution_count": null,
Expand All @@ -1293,7 +1318,7 @@
{
"data": {
"text/plain": [
"'/foo?a=&b=&c=1&c=2&d=bar'"
"'/foo?a=5&b=&c=1&c=2&d=bar&e='"
]
},
"execution_count": null,
Expand All @@ -1302,7 +1327,65 @@
}
],
"source": [
"qp('/foo', a=None, b=False, c=[1,2], d='bar')"
"qp('/foo', **vals)"
]
},
{
"cell_type": "markdown",
"id": "4740272f",
"metadata": {},
"source": [
"`qp` checks to see if it should be sent as a query parameter or as part of the route and encodes that properly."
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "ffbc0972",
"metadata": {},
"outputs": [
{
"data": {
"text/plain": [
"'/foo/5?b=&c=1&c=2&d=bar&e='"
]
},
"execution_count": null,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"qp('/foo/{a}',**vals)"
]
},
{
"cell_type": "markdown",
"id": "4c7eab51",
"metadata": {},
"source": [
"This works with multiple route parameters and query parmeter combinations"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "bd4d1630",
"metadata": {},
"outputs": [
{
"data": {
"text/plain": [
"'/foo/5/bar?b=&c=1&c=2&e='"
]
},
"execution_count": null,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"qp('/foo/{a}/{d}', **vals)"
]
},
{
Expand Down Expand Up @@ -1579,13 +1662,6 @@
"id": "e6ee3a86",
"metadata": {},
"outputs": [
{
"name": "stdout",
"output_type": "stream",
"text": [
"[Route(path='/foo', name='foo', methods=['GET', 'HEAD'])]\n"
]
},
{
"data": {
"text/plain": [
Expand All @@ -1602,7 +1678,30 @@
"@app.get\n",
"def foo(a:str, b:list[int]): ...\n",
"\n",
"print(app.routes)\n",
"foo.to(a='bar', b=[1,2])"
]
},
{
"cell_type": "code",
"execution_count": null,
"id": "9b9f1f03",
"metadata": {},
"outputs": [
{
"data": {
"text/plain": [
"'/foo/bar?b=1&b=2'"
]
},
"execution_count": null,
"metadata": {},
"output_type": "execute_result"
}
],
"source": [
"@app.get('/foo/{a}')\n",
"def foo(a:str, b:list[int]): ...\n",
" \n",
"foo.to(a='bar', b=[1,2])"
]
},
Expand Down Expand Up @@ -3260,6 +3359,83 @@
" 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 254d5e6

Please sign in to comment.