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

Figure out a better way to manage "current view" context #237

Open
taion opened this issue Feb 18, 2019 · 9 comments
Open

Figure out a better way to manage "current view" context #237

taion opened this issue Feb 18, 2019 · 9 comments

Comments

@taion
Copy link
Contributor

taion commented Feb 18, 2019

In many places, we have to explicitly pass the view into helper classes, e.g.

return self.filtering.filter_query(query, self)
. This is sort of clunky, and we end up passing those around in places where they may be unnecessary.

It would be better if we could bind the view object to helper class instances, or else have some flask_resty.current_view proxy.

@itajaja
Copy link
Member

itajaja commented Feb 19, 2019

should be fairly easy to register the view dispatch_request no?

@taion
Copy link
Contributor Author

taion commented Feb 19, 2019

yes, but that wouldn't let you do e.g. OtherView().query

@itajaja
Copy link
Member

itajaja commented Feb 19, 2019

yeah. maybe something like

class FooView(...):
    authorization = FooAuth()  # insert some python magic here that does `self.authorization.view = self`, I am pretty sure it's possible

eg in the view constructor /shrug

@taion
Copy link
Contributor Author

taion commented Feb 19, 2019

it works better if those are all classes, note. otherwise we'd be mutating objects or doing other magical things.

@sloria
Copy link
Contributor

sloria commented Feb 26, 2019

Yeah, marshmallow and WTForms mutate field objects upon instantiation (marshmallow via a Field #_bind_to_schema(field_name, schema) hook). Using classes would be better.

Perhaps deprecate authorization in favor of authorization_class (or authorization_classes if we want to allow a collection like in DRF) which could also support the proposal in #236?

class WidgetView(GenericModelView):
    authorization_class = WidgetAuthorization | AdminAuthorization

@taion
Copy link
Contributor Author

taion commented Feb 26, 2019

i'd call it Authorization to make inline defs a bit nicer-looking – class Authorization(FooView.Authorization) reads better than class authorization_class(FooView.authorization_class). we could take the same approach more broadly, though, e.g. with filtering, instead of doing:

filtering = Filtering(
    name=operator.eq,
)

we could have

class Filtering:
    name = operator.eq

would make it better to extend across views, too

class ExtendedView(BaseView):
    class Filtering(BaseView.Filtering):
        other_field = operator.eq

@itajaja
Copy link
Member

itajaja commented Feb 27, 2019

I thought magical things are the norm in python (sqlalchemy?) :P I am fine with inlining things but it's a big change in terms of project structuring, in the sense that right now things are separated in modules by types, whereas with this approach they will be separated by functionality

@sloria
Copy link
Contributor

sloria commented Feb 27, 2019

Yeah, I like the inlining idea in principle. Certainly a big change, though could be done iteratively. Also, views.py could get pretty huge...we might eventually want have a module per resource or even per view class.

@itajaja
Copy link
Member

itajaja commented Feb 27, 2019

depends how you use FR, if you are really high in the microservices spectrum, having every view in one file is very convenient. but if we look at our big services (mgmt), that retaliates.

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

No branches or pull requests

3 participants