Skip to content

Classbasedview#684

Merged
asvetlov merged 8 commits intomasterfrom
classbasedview
Dec 23, 2015
Merged

Classbasedview#684
asvetlov merged 8 commits intomasterfrom
classbasedview

Conversation

@asvetlov
Copy link
Copy Markdown
Member

@asvetlov asvetlov commented Dec 16, 2015

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
Copy Markdown
Member

fafhrd91 commented Dec 16, 2015

+1, but can we name it just View?

@asvetlov
Copy link
Copy Markdown
Member Author

asvetlov commented Dec 16, 2015

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

@jashandeep-sohi
Copy link
Copy Markdown
Contributor

jashandeep-sohi commented Dec 16, 2015

+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
Copy Markdown
Member Author

asvetlov commented Dec 16, 2015

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.

popravich
popravich reviewed Dec 17, 2015
View reviewed changes
examples/web_classview1.py
Copy link
Copy Markdown
Member

@popravich popravich Dec 17, 2015

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
Copy Markdown
Member Author

@asvetlov asvetlov Dec 17, 2015

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
Copy Markdown
Member

@popravich popravich Dec 17, 2015

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
Copy Markdown
Member

@popravich popravich Dec 17, 2015

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
Copy Markdown
Member Author

@asvetlov asvetlov Dec 17, 2015

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
Merge pull request #684 from KeepSafe/classbasedview
b06021f 
Classbasedview
@asvetlov asvetlov merged commit b06021f into master Dec 23, 2015
@asvetlov asvetlov deleted the classbasedview branch December 23, 2015 12:00
@milani
Copy link
Copy Markdown

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
Copy Markdown
Member Author

asvetlov commented May 16, 2018

  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
Copy Markdown

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
Copy Markdown
Member Author

asvetlov commented May 16, 2018

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
Copy Markdown

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.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@asvetlov @fafhrd91 @jashandeep-sohi @milani @popravich