allow 'name' in key word arguments of url_for() and url_path_for() (#608)

[dansan]

Patch replaces

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

with

def url_path_for(self, *args: str, **kwargs: str) -> URLPath:
    assert len(args) == 1, "Exactly one positional argument required: the routes name."
    name = args[0]

(also for url_for()) Not entirely sure about the assertion text...

Without the above change, these existing (now modified) tests fail:

• tests/test_routing.py::test_url_path_for
• tests/test_routing.py::test_url_for
• tests/test_routing.py::test_reverse_mount_urls

And this new one:

• tests/test_requests.py::test_request_url_for

[dansan]

Were multiple commits, because I had forgotten coverage and black. I squashed them together, thus the force-push.

[dansan]

I reverted the username → name changes, added a decorator to check the *args and added a test for the assertions. I didn't use the decorator in templating.py, because I'm unsure about importing issues/policies. (Wow - Travis is really clobbering me ;) Enforcing good standards and more automated QA... I think I want that at my work too...

[dansan]

Hi @blueyed , thanks for the review. I'm unsure about the procedure here. Github was nagging about the "Changes requested" by @tomchristie , so I clicked "request re-review" to get rid of it ^.^ although you're doing it now. Not intended as a rejection of your review, just to make github happy. I made the changes you requested.

[nwalsh1995]

Bumping this PR since I am affected by this same problem and would love to see this land!

[dansan]

I updated the code to be mergable to master.

[tomchristie]

Great stuff.

I'd probably prefer if we drop the verify_args_is_only_route decorator. The method on the Router class should make a proper type check. The Route/Mount/WebSocketRoute cases can just perform a dumber assert len(args) == 1 check.

[dansan]

I replaced the decorator with the asserts and the type check: f4f4cd1f7843e0e6f02fdc4b382facc671accea6

[fhscholl]

Any possibility this is going to get merged?

[Kludex]

We still have this "bug". Would you mind rebasing it? I know 2.5 years is a lot... But would you mind rebasing it @dansan ?

If not, I'd like to close the PR and creating an issue about this.

[dansan]

@Kludex Sorry, had no time before the holidays to get into this. Patch is rebased. Thanks for waking this old issue up.

[dansan]

@tomchristie could you approve the workflow to run?

[aminalaee]

@dansan Can you resolve the conflict please?

[dansan]

Rebased on master. Sorry for the delay.

[dansan]

Branch is up2date with encode/starlette→master.

[dansan]

Hum.. .I tried linking the PR to the existing issue (encode/starlette#608) by editing the commit message according to what is documented in https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue, but it's not working. The button for the manual way is not enabled for me.

... Ah - but after a while this PR is now linked in the issue. Maybe Github just needs a while to process this...

[dansan]

@tomchristie could you approve the workflow to start? Branch could be merged into master if the pipeline is successful. Thank you.

[Kludex]

I've set the milestone to 0.2x.x (the release after 0.20.0).

[adriangb]

My main issue with this is that using *args makes it harder for users to understand what the correct parameters are.

I'd think we'd want (/, name: str, **path_params: Any) -> URLPath or (name: str, path_params: Mapping[str, Any]) -> URLPath as our end state. The former will be possible in 2023 when 3.7 is EOL and the latter is possible now. But they are both breaking changes. So maybe as part of this PR we can choose our end goal/state and add a deprecation warning to start nudging us in that direction?

[Kludex]

Is it bad to replace name by __name? @adriangb

[adriangb]

Is it bad to replace name by __name? @adriangb

It means that anyone calling url_for(name="foo") where "foo" is the route name would experience a breaking change. But the same thing would happen with the current patch of name = args[0]. I think the only difference with the *args version is that the __name version (and subsequently the (/, name: str, ...)` version once Python 3.7 is our minimum version) put the intent in the function signature instead of burying it in the implementation.

[Kludex]

Can we deprecate the parameter "name"? Maybe warning when the caller uses "name=".

See:

import inspect


def potato(a: int, b: str) -> str:
    print(inspect.stack()[-1].code_context)
    return b


potato(1, b=2)

[alex-oleshkevich]

Can we deprecate the parameter "name"? Maybe warning when the caller uses "name=".

This is a good solution to have before 1.0. After we can have something like def url_for(self, name_:str, **path_params: Any) -> URL and clearly reserve name_ for internal use by documenting it.

[Kludex]

Is it bad to replace name by __name? @adriangb

It means that anyone calling url_for(name="foo") where "foo" is the route name would experience a breaking change. But the same thing would happen with the current patch of name = args[0]. I think the only difference with the *args version is that the __name version (and subsequently the (/, name: str, ...)` version once Python 3.7 is our minimum version) put the intent in the function signature instead of burying it in the implementation.

Let's take a decision here.

Options:

1. Go with this PR, and further revert + / on the signature.
2. Go with a less possible name like __name, and further addition of / to the signature.
3. ?

[abersheeran]

This PR may need to be considered in conjunction with #1385 .

There are several solutions to solve these two problems at the same time:

• def url_for(self, name: str, path_params: dict[str, Any], query_params: dict[str, Any]) -> str
• def url_for(self, *args: str, **kwargs: str) -> URL: Add query params to URL object.
• def url_for(self, *args: str, **kwargs: str) -> str: Unknown parameters in kwargs as query params. (like flask)
• def url_for(self, __name: str, **path_params: dict[str, Any], __query_params: dict[str, Any]) -> str
• def url_for(self, __name: str, **path_params: dict[str, Any]) -> URL

[Kludex]

I believe we prefer this one: def url_for(self, __name: str, **path_params: dict[str, Any]) -> URL.

[alex-oleshkevich]

Leading underscores are less discoverable. When you type n IDEs suggest you name parameter. When the parameter starts with _ then the autocompleting list if empty or not helpful.

What if we do like this def url_for(self, name_: str, **path_params: dict[str, Any]) -> URL. This is a far more common pattern I see in the codebases. And I don't remember if I ever seen name_ query parameter.

[Kludex]

The goal is to be less discoverable. :thinking:

The idea is to move to def url_for(self, name: str, /, **path_params: dict[str, Any]) -> URL when Python 3.7 support is no longer needed.

[Kludex]

I'm not sure if my last comment was that clear.

The goal of having __name is to signalize that we want it to be only a positional argument, so we can move to name: str, /, **path_params) later on.

[alex-oleshkevich]

There is no difference between __name or name_. But from DX perspective name_ is more clear.

[Kludex]

There is no difference between __name or name_. But from DX perspective name_ is more clear.

I see the goal here being exactly the opposite... Why would you want the IDE to show name__? People would get tempted to use the keyword and write url_for(name__="name"). I want to avoid people from writing the parameter so in a future release it can be removed. :thinking: