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

Fixed path_to_parts splitter #40

Merged
merged 14 commits into from
Jul 1, 2021
Merged

Conversation

kserhii
Copy link
Contributor

@kserhii kserhii commented Jun 9, 2021

This pull request fixes the path_to_parts helper method for such route

@api.route(
    r'/image/iiif/'
    r'<image_id>/'
    r'<region:full|square|\d+,\d+,\d+,\d+>/'
    r'<size:max|\d+,|,\d+|\d+,\d+>/'
    r'<rotation:int>/'
    r'default.jpg')

The idea for this solution was taken from the stackoverflow :)

@kserhii kserhii mentioned this pull request Jun 9, 2021
@ahopkins
Copy link
Member

ahopkins commented Jun 9, 2021

That is a slick solution. Will take a closer look and then also run it against the main repo. I keep meaning to add an action for that here.

@ahopkins
Copy link
Member

Might need some work on this to get all the tests passing. At least these tests on the main repo are failing:

  • tests/test_routes.py:1042 test_unicode_routes
  • tests/test_signals.py:13 test_add_signal
  • tests/test_signals.py:22 test_add_signal_decorator
  • tests/test_signals.py:53 test_dispatch_signal_triggers_multiple_handlers
  • tests/test_signals.py:75 test_dispatch_signal_triggers_triggers_event
  • tests/test_signals.py:110 test_dispatch_signal_triggers_with_requirements
  • tests/test_signals.py:127 test_dispatch_signal_triggers_with_context
  • tests/test_signals.py:142 test_dispatch_signal_triggers_with_context_fail
  • tests/test_signals.py:157 test_dispatch_signal_triggers_on_bp
  • tests/test_signals.py:186 test_dispatch_signal_triggers_event
  • tests/test_signals.py:212 test_dispatch_signal_triggers_event_on_bp
  • tests/test_signals.py:244 test_bad_finalize
  • tests/test_signals.py:275 test_event_not_exist_with_autoregister
  • tests/test_signals.py:284 test_dispatch_signal_triggers_non_exist_event_with_autoregister
  • tests/test_signals.py:306 test_dispatch_not_exist
  • tests/test_signals.py:330 test_signal_reservation[foo.bar.baz-True]
  • tests/test_signals.py:330 test_signal_reservation[server.init.before-False]
  • tests/test_signals.py:330 test_signal_reservation[http.request.start-False]
  • tests/test_signals.py:330 test_signal_reservation[sanic.notice.anything-True]

I think the signals should be an easy fix by changing the delimiter from . to \.. But I am not sure what is going on with the unicode routes yet.

@kserhii
Copy link
Contributor Author

kserhii commented Jun 15, 2021

I was a bit confused about where those tests are.
But then I found them in the Sanic project :)
Now, this solution should work for dot delimiter as well

@ahopkins
Copy link
Member

Great, thanks. Can you add a test in here as well with some unicode tests? The Sanic core tests use: "/你好"

sanic_routing/utils.py Outdated Show resolved Hide resolved
@kserhii
Copy link
Contributor Author

kserhii commented Jun 29, 2021

Done!
Please check with Sanic tests

@ahopkins
Copy link
Member

ahopkins commented Jul 1, 2021

All good 🎉

@ahopkins ahopkins merged commit 1cc4b73 into sanic-org:main Jul 1, 2021
@kserhii kserhii deleted the fix-path-splitter branch July 1, 2021 18:34
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.

None yet

2 participants