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

Route definitions #2004

Merged
merged 31 commits into from
Aug 4, 2017
Merged

Route definitions #2004

merged 31 commits into from
Aug 4, 2017

Conversation

asvetlov
Copy link
Member

Alternative to #2003

@fafhrd91 your proposal was very seductive.

I've implemented it :)

@asvetlov
Copy link
Member Author

The usage:

    routes = web.RouteDef()

    @routes.get('/path')
    @asyncio.coroutine
    def handler(request):
        pass

    app.router.add_routes(routes)

@fafhrd91
Copy link
Member

cool!

what if Routes behave more like sub app, then you can manage set of routes as one entity and apply rules to all routes at the same time

@asvetlov
Copy link
Member Author

I don't follow.
What rules are you talking about?

@codecov-io
Copy link

codecov-io commented Jun 22, 2017

Codecov Report

Merging #2004 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2004      +/-   ##
==========================================
+ Coverage   97.09%   97.16%   +0.06%     
==========================================
  Files          39       39              
  Lines        7781     7967     +186     
  Branches     1357     1389      +32     
==========================================
+ Hits         7555     7741     +186     
- Misses        101      102       +1     
+ Partials      125      124       -1
Impacted Files Coverage Δ
aiohttp/web_urldispatcher.py 99.48% <100%> (+0.05%) ⬆️
aiohttp/test_utils.py 99% <0%> (+0.39%) ⬆️
aiohttp/web_fileresponse.py 91.3% <0%> (+0.82%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fa0ef99...c10f4e0. Read the comment docs.

@fafhrd91
Copy link
Member

for example, url prefix, or some kind of permissions, etc. or even use different route style.
maybe it is too far and too broad

@asvetlov
Copy link
Member Author

asvetlov commented Jun 22, 2017

Right now we have no permissions, let's don't discuss it here.
Regarding to url prefix -- it's doable but have own drawback.
Let's consider the following:

    routes = web.RoutesDef()

    @routes.get('/path')
    @asyncio.coroutine
    def handler(request):
        pass

    app.router.add_routes(routes, prefix='/base')

Final url will be '/base/path'. The behavior is pretty simple and obvious.
But my guts feel this is not very good idea.
With RoutesDef the route table is scratched over many places in many files.
The most practical way to find proper web handler is grepping the source code by URL.
Prefixes make the process much harder: route definitions may have the same path but different prefixes, it should very common for REST based solutions.
Thus better to have longer paths in decorators for easy searching. Otherwise user should look on two places: decorator and RoutesDef registration.

P.S.
I prefer RoutesDef to Routes: we already have Route concept and it's used for registered route. But RoutesDef is for unregistered route definition, let's don't mix them.

Now I think RouteInfo namedtuple should be renamed to RouteDef (the name will almost newer be used in application code).
More important router.add_routes() should renamed .add_routes_def(). It's 4 chars longer but supports more native semantics.

@fafhrd91
Copy link
Member

I think of RoutesDef as common context for all routes. same as sub application. without common context it becomes just list of routes and what is the point then introduce new concept

@asvetlov
Copy link
Member Author

Sub-application has a context: state aka app['name'], own signals and middlewares. Why do we need another stateful concept?
RoutesDef in opposite is just a state-less list of route definitions but with convenient methods like routes.get(...) for usage as web-handler decorators.

@asvetlov asvetlov changed the title Route definitions (secoond attempt) [WIP] Route definitions (second attempt) Jun 22, 2017
@asvetlov
Copy link
Member Author

asvetlov commented Jun 23, 2017

it create some hidden module level variable, put route data in it. Then routes method, something like load_mod(module object) reads variable and register routes

This is a movement in opposite direction than your original propose.

  1. Adding a hidden module variable increases a level of hidden magic a bit. I cannot name the solution as very explicit.
  2. Another problem is: how to determine a modude which should be used by decorator?

At first glance it could be sys.modules[handler.__module__] and this works for 99% use cases but silently fails if the handler is defined in one module but decorated in other like handler = web.get('/path')(handler). It's very unusual and code looks weird but and don't want to add this kind of weird error-by-design at all, it's very not pythonic. Looks like design bomb.

Fetching data by analyzing sys._getframe() frames is even worse by very many reasons: user written helpers, pypy support, cython support and I dont know what else potential errors.

P.S.
RoutesDef instance might be shared between modules if user found it convenient to him -- why not.
Like not all Flask's blueprints and limited to single module only -- it might be a package also with routes scattered by package modules.

P.P.S.
app.router.load_mod(module object) doesn't look pretty for single file projects. __main__ module has no a reference to itself, the only way to make a call is app.router.load_mod(sys.modules['__name__']). Not very elegant solution.

@asvetlov asvetlov added this to the 2.3.0 milestone Jun 23, 2017

HTTP_METHOD_RE = re.compile(r"^[0-9A-Za-z!#\$%&'\*\+\-\.\^_`\|~]+$")
PATH_SEP = re.escape('/')


class RouteDef(namedtuple('_RouteDef', 'method, path, handler, kwargs')):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not only Route?
What's the meaning of Def?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because we already use Route name for routes added by app.router.add_route() call.
RouteDef is a scratch for non-added-yet route definition



def head(path, handler, **kwargs):
return route(hdrs.METH_HEAD, path, handler, **kwargs)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the code gets more readable when we do not use abbreviations. Can I open a PR by renaming hdrs to headers?

Copy link
Member Author

Choose a reason for hiding this comment

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

It would be good in general but for this particular case it doesn't make sense I think.
hdrs is not a part of our public API anyway.

route.register(self)


def route(method, path, handler, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

i think after refactoring this function is not required

Copy link
Member Author

Choose a reason for hiding this comment

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

It is: user might want to provide custom HTTP method in very rare cases


HTTP_METHOD_RE = re.compile(r"^[0-9A-Za-z!#\$%&'\*\+\-\.\^_`\|~]+$")
PATH_SEP = re.escape('/')


class RouteDef(namedtuple('_RouteDef', 'method, path, handler, kwargs')):
# TODO: add __repr__
Copy link
Member

Choose a reason for hiding this comment

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

def __repr__(self):
    return (
        "<RouteDef {method} {path} -> {handler.__name__!r}, "
        "{kwargs}>".format(**self._asdict())
    )
def __str__(self):
    return self.__repr__()
__________
result

<RouteDef GET /some -> 'get_user', {}>

but i think will be better just Route

Copy link
Member Author

Choose a reason for hiding this comment

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

As I said Route name is captured by already existing class hierarchy.
It's present instantiated route.
In opposite route definition is for list of routes to be added to router instance.

I still think the distinction is matter.

def __init__(self):
self._items = []

# TODO: add __repr__
Copy link
Member

Choose a reason for hiding this comment

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

import operator

def __repr__(self):
    items = sorted(
        self._items,
        key=operator.attrgetter('path', 'method')
    )
    return "<RoutesDef\n\t{}\n>".format(
        "\n\t".join(map(str, items))
    )

____
result

<RoutesDef
	< RouteDef GET /article -> 'get_article', {}>
	< RouteDef GET /user -> 'get_user', {}>
	< RouteDef POST /user -> 'post_user', {}>
>

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for suggestion for route definitions should be sorted by insertion order.
It is crucial for router implementation.

@asvetlov
Copy link
Member Author

asvetlov commented Aug 1, 2017

@fafhrd91 and others.

I love to merge the PR after polishing.

Now we have directive way for adding new routes: app.router.add_get(...)
The PR proposes router.add_routes() for adding routes in Django style: https://github.com/aio-libs/aiohttp/pull/2004/files#diff-d24e0645e091e8104b32633c7a3cab93
Looks like nobody objects.

Also it adds decoration based style like flask and bottle: https://github.com/aio-libs/aiohttp/pull/2004/files#diff-1fe46683db6b1c2e6b977a6b3f5da19f
The only difference the PR introduces RoutesDef class (maybe RouteTableDef is better name) for collecting routes by decorator appliance and adding them to router explicitly.

I feel people need it too.
I did my best to keep aiohttp spirit: everything is explicit, no magic even if users should write more code.
No side effects and potential break downs.

If @fafhrd91 will say strict no for PR I'll split it into separate ones for table and decorators.
But please don't do it, I very want to have both features in next aiohttp release.

@fafhrd91
Copy link
Member

fafhrd91 commented Aug 1, 2017

lgtm

@asvetlov
Copy link
Member Author

asvetlov commented Aug 1, 2017

Thank you @fafhrd91

@asvetlov asvetlov changed the title [WIP] Route definitions (second attempt) Route definitions Aug 2, 2017
@asvetlov
Copy link
Member Author

asvetlov commented Aug 2, 2017

BTW the PR is pretty close to https://github.com/IlyaSemenov/aiohttp_route_decorator

@asvetlov
Copy link
Member Author

asvetlov commented Aug 2, 2017

Docs added, code is 100% covered.
The PR is done.
Will merge it after tomorrow if nobody object.

info += ", {}={}".format(name, value)
return ("<RouteDef {method} {path} -> {handler.__name__!r}"
"{info}>".format(method=self.method, path=self.path,
handler=self.handler, info=''.join(info)))
Copy link
Member

Choose a reason for hiding this comment

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

i think will be better if info will be string
then you can delete ''.join(info)

Copy link
Member Author

Choose a reason for hiding this comment

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

This style is used by asyncio in many places, I like to keep it in aiohttp too

@asvetlov asvetlov merged commit 8dbad8c into master Aug 4, 2017
@asvetlov asvetlov deleted the route_deco2 branch August 4, 2017 08:13
@lock
Copy link

lock bot commented Oct 28, 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 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019
@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Oct 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bot:chronographer:provided There is a change note present in this PR outdated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants