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

Classbasedview #684

Merged
merged 8 commits into from
Dec 23, 2015
Merged

Classbasedview #684

merged 8 commits into from
Dec 23, 2015

Conversation

asvetlov
Copy link
Member

I've added example for Class Based View.
It doesn't require any aiohttp code change for async def/await approach but one line change is needed for @coroutine/yield from` old style.

Folks, do you think BaseView matters to be included into aiohttp.web itself?

@fafhrd91
Copy link
Member

+1, but can we name it just View?

@asvetlov
Copy link
Member Author

Ok. Django use View as base class for class-based-views also.

@jashandeep-sohi
Copy link
Contributor

+1 for including something like this in aiohttp.web.

I want to use it for the static file handler refactor, and I'd rather not implement it again.
I was planing on adding something that can be used like this:
router.add_route("*", "/fake/path/to/file", FileHandler("/real/path/to/file.ext"))

However, I don't like the idea of creating a new instance for every incoming request. Couldn't this be implemented somehow using the same approach as Custom Routing Criteria and a __call__?

@asvetlov
Copy link
Member Author

There are two kinds of views organized in classes:

  • registered as method of class instance like proposed in Custom Routing Criteria
  • registered as class itself (not instance) as the PR suggests

First way doesn't need any library change. Just add __call__ coroutine accepting request as you mentioned. Clean and obvious, isn't it?
For second one standard base view is desired.

app = Application(loop=loop)
app.router.add_route('GET', '/', index)
app.router.add_route('GET', '/get', MyView)
app.router.add_route('POST', '/post', MyView)
Copy link
Member

Choose a reason for hiding this comment

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

So what's the profit?

  1. You still need to register routes by hand (not auto-discovered by router).
  2. The MyView class is used not instance of MyView, so no custom initialization could be done. This would lead to some "view factories" like:
def my_view_factory(*args, **kw):
    return lambda request: MyView(request, *args, **kw)

Copy link
Member Author

Choose a reason for hiding this comment

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

People asked for "class based views like Django" many times.
Personally I prefer another style of views organizing like http://aiohttp.readthedocs.org/en/stable/web.html#using-plain-coroutines-and-classes-for-web-handlers you know.

  1. Views should not be autodiscovered (but you may use '*' as method name).
  2. Yes, in non-trivial cases user should use custom factories (just like for asyncio protocols). I don't want to make a mess adding clumsy initializers like django's View.as_view() etc.

Copy link
Member

Choose a reason for hiding this comment

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

ok, agree to 1.
2. looks like a next request after this one.

Anyway, I not sure which is worse -- to have "class based views like Django" in aiohttp.web or to let people reinvent the wheel themselfs...

Copy link
Member

Choose a reason for hiding this comment

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

If people asked me the question I would try to explain why they don't need it

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is in this line: https://github.com/KeepSafe/aiohttp/pull/684/files#diff-b41dc5c66bc19f22fe7cc77c002db02cR501

Without it people may do it themselves (while most of them not experienced enough to override __iter__ or __await__ properly).

asvetlov added a commit that referenced this pull request Dec 23, 2015
@asvetlov asvetlov merged commit b06021f into master Dec 23, 2015
@asvetlov asvetlov deleted the classbasedview branch December 23, 2015 12:00
@milani
Copy link

milani commented May 16, 2018

Why is it not named Controller? Not knowing Django, I had a hard time understanding this concept from the documents since View is something to render the view. But here, it is used for routing or doing business logic. Since aiohttp is not going to dictate the structure (MTV vs MVC), I think another naming is necessary to avoid confusion.

@asvetlov
Copy link
Member Author

  1. The ship has sailed 2.5 years ago.
  2. Controller is a name as confusing as View at least for people who don't know Ruby-on-Rails.

@milani
Copy link

milani commented May 16, 2018

  1. I know. But the only way I could clear my doubts was to read the code. So I am here.
  2. The point is that there is no reason to prefer one above the other. Using Controller or View imposes a specific structure. So I guess another name (like Route, if it is not taken) is less confusing.

@asvetlov
Copy link
Member Author

Route is taken for route declaration, unfortunately. flask` has class based views also.
Looks like people a familiar with the concept (at least nobody objected except you).
API renaming is always a pain, need a very strong reason for it.

@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].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

@lock lock bot added the outdated label Oct 28, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 28, 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.

5 participants