Robyn
Robyn copied to clipboard
Working template get_function_url with test
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 get_function_url 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 get_function_url with the
sync_authfunction - test_get_requests.py (add an assert to check for
/sync_authin 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
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 | Jan 9, 2025 4:52pm |
CodSpeed Performance Report
Merging #1004 will not alter performance
Comparing dave42w:url_for (2ce9b02) with main (2aad774)
Summary
✅ 138 untouched benchmarks
I'm stuck. See https://github.com/sparckles/Robyn/issues/996#issuecomment-2451630716
Hey @dave42w 👋
Is it up for review??
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 :-)
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
Hi @sansyrox
I've sorted out the pyproject.toml problem (a) :-)
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
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 😄
@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?
Hi @sansyrox I think this is ready, just need to confirm that you are happy with my responses.
Hi @sansyrox I wanted to check if you are happy for the conversations to be marked as resolved?
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?
Hi @sansyrox After the CI updates this is passing all tests. Can we merge it please. Thanks
Hey @dave42w 👋
Thanks for reminding here. I will review this over the weekend :D
⚠️ Only 5 files will be analyzed due to processing limits.
😱 Found 1 issue. Time to roll up your sleeves! 😱