Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

"url_for" signature prevents use of "name" in path arguments #608

Closed
dansan opened this issue Aug 21, 2019 · 5 comments · Fixed by #2050
Closed

"url_for" signature prevents use of "name" in path arguments #608

dansan opened this issue Aug 21, 2019 · 5 comments · Fixed by #2050

Comments

@dansan
Copy link
Contributor

dansan commented Aug 21, 2019

I'm implementing an API in Fastapi, using APIRouters for different resources.
When I want to get the URL of an item in a resource, I use the starlette.requests.Request objects url_for() method.
When I have a function with a path /resource/{name} I cannot use url_for() to get the URL, because it will result in:

TypeError: url_for() got multiple values for argument 'name'

Reproduce with:

def func(name, **path_params):
    pass
func("foo", name="bar")

As a workaround I have renamed the path variable in my API, but name was really fitting, and what I wanted to use, and not at all an uncommon variable name.
From what I see, name in url_for() seems to be the name of the referenced method in APIRouter. Changing the signature of url_for() to use router_method instead of name, would make a collision with the key word arguments in path_params less likely. Result would be:

def url_path_for(self, router_method: str, **path_params: str) -> URLPath:

An inspection on the used arguments could be used to log a deprecation warning until the next release, when url_path_for() is used with a named name=... instead of positional.

If this proposal is acceptable, I'd create a PR.

@tomchristie
Copy link
Member

I’d suggest we change it to an unnamed argument. We could do this by changing the argument signature to *args, **kwargs, and ensuring that len(args) == 1

@dansan
Copy link
Contributor Author

dansan commented Aug 22, 2019

PR: #611

dansan added a commit to dansan/starlette that referenced this issue Aug 22, 2019
dansan added a commit to dansan/starlette that referenced this issue Aug 22, 2019
dansan added a commit to dansan/starlette that referenced this issue Sep 13, 2019
dansan added a commit to dansan/starlette that referenced this issue Nov 19, 2019
dansan added a commit to dansan/starlette that referenced this issue Nov 28, 2019
@dansan
Copy link
Contributor Author

dansan commented Jan 1, 2022

Rebased on top of current master.

@chbndrhnns
Copy link

I was hit by that issue just today and would like to see it merged.

@tzmara

This comment was marked as spam.

keul added a commit to ecmwf-projects/cads-catalogue-api-service that referenced this issue Mar 17, 2023
This has been required by changes in starlette. See encode/starlette#608
cettina-tosto pushed a commit to ecmwf-projects/cads-catalogue-api-service that referenced this issue Mar 24, 2023
This has been required by changes in starlette. See encode/starlette#608
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants