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

Working template get_function_url with test #1004

Open
wants to merge 36 commits into
base: main
Choose a base branch
from

Conversation

dave42w
Copy link
Contributor

@dave42w dave42w commented Oct 28, 2024

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 #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_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

Copy link

vercel bot commented Oct 28, 2024

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 Dec 31, 2024 7:31am

Copy link

codspeed-hq bot commented Oct 28, 2024

CodSpeed Performance Report

Merging #1004 will not alter performance

Comparing dave42w:url_for (2ce9b02) with main (2aad774)

Summary

✅ 138 untouched benchmarks

@dave42w
Copy link
Contributor Author

dave42w commented Nov 1, 2024

I'm stuck. See #996 (comment)

@dave42w dave42w changed the title Initial static text url_for implementation Working Jinja2 url_for with tests Nov 3, 2024
@dave42w dave42w changed the title Working Jinja2 url_for with tests Working Jinja2 url_for with test Nov 3, 2024
…n. ruff fixes. Safety check for no robyn object.
@dave42w
Copy link
Contributor Author

dave42w commented Nov 24, 2024

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?

@dave42w dave42w changed the title Working Jinja2 url_for with test Working template get_function_url with test Dec 6, 2024
@dave42w
Copy link
Contributor Author

dave42w commented Dec 20, 2024

Hi @sansyrox
After the CI updates this is passing all tests. Can we merge it please.
Thanks

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.

2 participants