Working Jinja2 url_for with test

dave42w commented 4 weeks ago

Description

This PR starts the process of implementing url_for so that Jinja templates can reference Robyn endpoints by function name rather than path see https://github.com/sparckles/Robyn/issues/996

Summary

[Updated] ~~This PR adds a url_for that returns a fixed string and a test for this.~~ 3rd Nov 24 Now looks up functions via the Robyn object router 7th Nov. All finished.

Changes are limited to 7 files (5 of them related to testing)

templating.py (define the url_for function and add it to the global Jinja environment using generic add_function_to_globals())
base_routes.py (call the set_robyn method of the JinjaTemplate)
template.html (display test output of url_for with the sync_auth function
test_get_requests.py (add an assert to check for /sync_auth in an href)
test_get_function_url (unit tests)
test_get_param_filled_url (unit tests)

~~TODO handle positional and named arguments~~ ~~TODO add set_robyn and url_for to template_interface~~ ~~TODO warn if no Robyn object set using set_robyn~~ TODO improve response to an invalid route ~~TODO rename from url_for to get_function_url in preparation for adding get_static_url~~ ~~TODO remove @pytest.mark.url_for from test_get_requests.py`~~ ~~jinja_template = JinjaTemplate(".", "utf-8")~~ #solves my git locale pre-commit issue ~~TODO add unit tests~~ ~~TODO add unit tests that cover combinations of url's~~ TODO documentation

PR Checklist

Please ensure that:

[X ] The PR contains a descriptive title
[ X] The PR contains a descriptive summary of the changes
[ X] You build and test your changes before submitting a PR.
[ ] You have added relevant documentation (I've added a docstring, until we get to the point where it can be used in a functional way I've not added it to the main documentation)
[ X] You have added relevant tests. We prefer integration tests wherever possible

Pre-Commit Instructions:

[ ] Ensure that you have run the pre-commit hooks on your PR. I think I've done that. I'm running the tests from a separate clone which I have had to modify to work with Python 3.12.7

vercel[bot] commented 4 weeks ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
robyn	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Nov 24, 2024 9:06pm

codspeed-hq[bot] commented 4 weeks ago

CodSpeed Performance Report

Merging #1004 will not alter performance

_{Comparing dave42w:url_for (d0d8e7b) with main (abb42d0)}

Summary

✅ 146 untouched benchmarks

dave42w commented 3 weeks ago

I'm stuck. See https://github.com/sparckles/Robyn/issues/996#issuecomment-2451630716

sansyrox commented 2 weeks ago

Hey @dave42w 👋

Is it up for review??

dave42w commented 2 weeks ago

Hey @dave42w 👋

Is it up for review??

Hesitant yes. I only commit when it passes all the tests and works to a completed step.

a) by mistake I committed local changes to pyproject.toml that are needed for using uv in my python 3.12.7 environment. I'm wondering if I need to chuck this PR, create a new branch and recommit my changes without pyproject.toml?

b) this works and the tests pass but it doesn't yet support keyword parameters (eg from the examples /crime/:crime_id). I'm working on that (got distracted by drinking wine to hide from a certain set of election results). I've worked out how to do it and it's not a big job so should be done by the end of Saturday. Did you see my discord question about whether there are any restrictions on parameters to url's? It shouldn't affect this code apart from more robust error checking see https://discord.com/channels/999782964143603713/999782965460603056/1303425727227756595

c) I would appreciate feedback on what to return for error conditions (eg no call to set_robyn, no route found) I'm trying to balance informative with not leaking info useful to someone trying to penetrate the system. In many ways I'd like to use a standard httpStatus but not sure how that would work and if it would make debugging very hard (if we had a standard static 404 url I could return that).

d) I would appreciate feedback on the issue of static files see https://github.com/sparckles/Robyn/issues/1005 I have changed the function name that goes into the template in line with that thinking (it makes it much simpler IMHO to have separate functions to use in the template for static files and for functions rather than using a "static" argument to url_for as flask does).

So short answer, feedback would be great :-)

dave42w commented 2 weeks ago

Hi @sansyrox

I've just added the support for URL parameters. All committed with tests.

So from my previous comment today b) is done. Feedback on a) c) and d) appreciated.

Thanks

Dave

dave42w commented 2 weeks ago

Hi @sansyrox

I've sorted out the pyproject.toml problem (a) :-)

dave42w commented 2 weeks ago

Hey @dave42w 👋

Is it up for review??

Hi,

Wondering if you are able to do that review or if there is more I need to do.

Thanks

sansyrox commented 2 weeks ago

Hey @dave42w 👋

Apologies for the super late review. My work has been mental. I will try to complete my review by tonight or tomorrow for sure 😄

dave42w commented 1 week ago

@sansyrox I haven't marked the 2 conversations as resolved as I figured you need to be happy with my responses first. Is that the right approach?

dave42w commented 1 week ago

Hi @sansyrox I think this is ready, just need to confirm that you are happy with my responses.

dave42w commented 5 days ago

Hi @sansyrox I wanted to check if you are happy for the conversations to be marked as resolved?

dave42w commented 1 day ago

Hi, @sansyrox

I think the test failures are CI timeouts, this has passed all the tests at some point since the last change.

Can we merge?

sparckles / Robyn