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.

[lovelydinosaur]

Lovely stuff - couple of minor changes suggested - otherwise looks ace!

[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.

[lovelydinosaur]

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: f4f4cd1

[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.

[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. 🤔

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. 🤔

[Kludex]

Is it possible to have a different signature depending on the Python version? Maybe we should already do this...

[alex-oleshkevich]

Is it possible to have a different signature depending on the Python version? Maybe we should already do this...

I am aware that it could be an unexpected side effect when devs will update the project's python version.
Maybe we should enforce it at mypy level at this state?

[Kludex]

Is it possible to have a different signature depending on the Python version? Maybe we should already do this...

I am aware that it could be an unexpected side effect when devs will update the project's python version. Maybe we should enforce it at mypy level at this state?

You mean this?

import typing


if typing.TYPE_CHECKING:
    def potato(__name: str, /) -> str:
        """Potato."""
        return __name
else:
    def potato(__name: str) -> str:
        """Potato."""
        return __name

potato(__name="potato")  # ERROR HERE

It's not possible due to syntax error. 😢

[Kludex]

Anyone subscribed here is against the __name proposal with further addition of positional-only argument on __name (when we drop support for Python 3.7)?

[Kludex]

• Closed by Allow "name" argument on url_for() and url_path_for() #2050