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

Allow "name" argument on templates url_for() #2127

Merged
merged 2 commits into from
May 31, 2023
Merged

Allow "name" argument on templates url_for() #2127

merged 2 commits into from
May 31, 2023

Conversation

rosstitmarsh
Copy link
Contributor

@rosstitmarsh rosstitmarsh commented Apr 20, 2023

Summary

#2050 fixed the url_for functions not allowing "name" as a parameter, but it missed the templating url_for function

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Apr 20, 2023

Hmm... Not sure if we should do this... The __name should have been the first parameter.

@rosstitmarsh
Copy link
Contributor Author

Hmm... Not sure if we should do this... The __name should have been the first parameter.

How do you mean? The @pass_context decorator inserts context as the first argument.

@Kludex Kludex requested a review from aminalaee April 27, 2023 23:40
Copy link
Member

@aminalaee aminalaee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I tested manually but maybe worth adding a simple test case.

@adriangb
Copy link
Member

At this point, why don’t we wait a bit and drop support for 3.7 first? Then we can make it a runtime enforced positional argument.

@Kludex
Copy link
Member

Kludex commented Apr 30, 2023

At this point, why don’t we wait a bit and drop support for 3.7 first? Then we can make it a runtime enforced positional argument.

But the context parameter shouldn't be... 🤔

@adriangb
Copy link
Member

Not sure what you mean. But also re-reading I realized we already merged a sibling PR, so no point in holding off.

@rosstitmarsh
Copy link
Contributor Author

rosstitmarsh commented May 2, 2023

LGTM. I tested manually but maybe worth adding a simple test case.

This was the test I made to make sure my change worked. I didn't add it to the PR because it seemed a bit trivial for something that will be redundant soon. I will add it if you want.

def test_template_name_parameter(tmpdir, test_client_factory):
    path = os.path.join(tmpdir, "index.html")
    with open(path, "w") as file:
        link = "<a href='{{ url_for('homepage', name=name) }}'>{{ name }}</a>"
        file.write(f"<html>Hello, { link }</html>")

    async def homepage(request):
        return templates.TemplateResponse(
            "index.html", {"request": request, "name": request.path_params["name"]}
        )

    app = Starlette(
        debug=True,
        routes=[Route("/{name}", endpoint=homepage)],
    )
    templates = Jinja2Templates(directory=str(tmpdir))

    client = test_client_factory(app)
    response = client.get("/world")
    exptected_html = "<html>Hello, <a href='http://testserver/world'>world</a></html>"
    assert response.text == exptected_html
    assert response.template.name == "index.html"
    assert set(response.context.keys()) == {"request", "name"}

@Kludex
Copy link
Member

Kludex commented May 2, 2023

My point is... There's a parameter context before name, and the idea on the other PR was to rename name by __name because the IDEs ignore it, but also as a first step towards positional-only argument, which is not possible on the function on this PR.

@Kludex Kludex requested a review from aminalaee May 27, 2023 08:44
@Kludex
Copy link
Member

Kludex commented May 31, 2023

Ah, my bad. I was actually saying nonsense.

@Kludex Kludex enabled auto-merge (squash) May 31, 2023 15:56
@Kludex Kludex merged commit 5898f21 into encode:master May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants