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

Add possibility to register a regexp in router. #291

Merged
merged 3 commits into from
Mar 14, 2015

Conversation

klen
Copy link
Contributor

@klen klen commented Mar 13, 2015

I've added possibility to register any regexp in aiohttp.web.UrlDispatcher. For that moment there is no opportunity to create some dynamic routes like /test/(?P<any_include_slashes>*.) or maybe something like this /test/(.*)/?. The little request implement the functionality.

# common aiohttp usage
app.router.add_route('GET', '/some', handler)

import re

# Define regexp manually
app.router.add_route('GET', re.compile('^/any/regexp/here/?$'), handler)

@asvetlov
Copy link
Member

-1

You already can use construction like:

self.router.add_route('GET', r'/handler/{to:.+}/tail', handler)

@asvetlov asvetlov closed this Mar 13, 2015
@klen
Copy link
Contributor Author

klen commented Mar 13, 2015

No, I can't.

What should I do if I need regexp like this by example:

/resource/([^/]+)?/?

Looks like current solution is:

 handler = ...
 app.router.add_route('GET', '/resource', handler)
 app.router.add_route('GET', '/resource/', handler)
 app.router.add_route('GET', '/resource/{resource}', handler)
 app.router.add_route('GET', '/resource/{resource}/', handler)

Of course for current version users could do that:

from aiohttp.web import DynamicRoute
import re

pattern = re.compile('...')
app.router._register_endpoint(DynamicRoute('GET', pattern.pattern, handler, pattern))

But it's little bit ugly. Much better app.router.register('GET', re.compile('...')) isnt?

What the problem to provide for users more control, which breaks nothing?

@asvetlov
Copy link
Member

At least it breaks reverse url building by route.url() call.
Regexp is not a formatter string.

@asvetlov
Copy link
Member

Also, what the difference between /resource/([^/]+)?/? and r'/handler/{tail:([^/]+)?/?}

@klen
Copy link
Contributor Author

klen commented Mar 13, 2015

At least it breaks reverse url building by route.url() call.

I have not thought about route.url, it could be problem coz. But I see some solutions for this.

Also, what the difference between /resource/([^/]+)?/? and r'/handler/{tail:([^/]+)?/?}

Second pattern doesn't work in current version aiohttp (because https://github.com/KeepSafe/aiohttp/blob/master/aiohttp/web.py#L1448). So that the main reason why I've made the request for support custom regexp.

@asvetlov
Copy link
Member

Aha, I see.
I'm strongly -1 for pushing regexp instances into .add_route() but ok to relax route pattern rules to pass regex with slashes (IIRC Pyramid allows url patterns like this).

@asvetlov asvetlov reopened this Mar 13, 2015
@klen
Copy link
Contributor Author

klen commented Mar 13, 2015

And looks like route.url is already broken:

from aiohttp.web import UrlDispatcher as Router

route = Router().add_route('GET', r'/{one}/{two:.+}', lambda r: None)

# will raise ValueError
route.url({'one': 1, 'two': 2})

@asvetlov
Copy link
Member

Sure, it works:

def test_route_dynamic_with_regex(self):
    handler = self.make_handler()
    route = self.router.add_route('GET', r'/{one}/{two:.+}', handler)

    url = route.url(parts={'one': 1, 'two': 2})
    self.assertEqual('/1/2', url)

@klen
Copy link
Contributor Author

klen commented Mar 13, 2015

Oh, I have look to version in pypi before #264

@klen
Copy link
Contributor Author

klen commented Mar 13, 2015

Ok, please have a look on the last solution. It makes the deal with slashes as well and pass all tests except this one:

def test_add_url_invalid5(self):
    handler = self.make_handler()
    with self.assertRaises(ValueError):
        self.router.add_route('post', '/post"{id}', handler)

Because the new dispatcher allows to define routes like this as dynamic. route.url(parts={'id': 1}) -> /post"1

"Bad pattern '/handler/(?P<to>+++)': nothing to repeat"), s)
self.assertEqual(
"Bad pattern '\/handler\/(?P<to>+++)': nothing to repeat",
str(ctx.exception))
Copy link
Member

Choose a reason for hiding this comment

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

Please revert the test: on Python 3.5 error message contains additional info about position in text which causes the error.

@asvetlov
Copy link
Member

The last patch if perfect except my tiny inline notes.

@asvetlov
Copy link
Member

Hmm. Does you patch violate the rule "A pattern segment (an individual item between / characters in the pattern) may either be a literal string (e.g. foo) or it may be a replacement marker (e.g. {foo}) or a certain combination of both"?

@asvetlov
Copy link
Member

Please forget about last message: I want to support patterns like foo/{name}.html too.

asvetlov added a commit that referenced this pull request Mar 14, 2015
Add possibility to register a regexp in router.
@asvetlov asvetlov merged commit 5269480 into aio-libs:master Mar 14, 2015
@asvetlov
Copy link
Member

Thanks!

@asvetlov asvetlov mentioned this pull request Dec 22, 2015
@lock
Copy link

lock bot commented Oct 30, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 30, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 30, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants