[GitHub] [incubator-superset] willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization
willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization URL: https://github.com/apache/incubator-superset/issues/9077#issuecomment-588373465 @john-bodley My understanding here is a little fuzzy too, so I'll let @dpgaspar correct me if I'm wrong. FAB creates blueprints under the hood whenever we use it to define endpoints. `ModelView` and `ModelViewApi` classes create collections of endpoints under a blueprint, which is then registered on the application when we call `appbuilder.add_view...` or a similar function. If we wanted to group those items together into an API blueprint, it would not be possible since that would result in nested blueprints. @dpgaspar how far away from the truth am I :)? This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization
willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization URL: https://github.com/apache/incubator-superset/issues/9077#issuecomment-587924995 @john-bodley I didn't address your comment directly regarding blueprints. Let me do so now: I concur 100% with the idea of untangling the `app` references from the rest of the code. The problem we currently face with regards to blueprints is that: 1. Blueprints cannot currently be nested (see [this issue on Flask](https://github.com/pallets/flask/issues/593)) 2. Flask-AppBuilder (FAB) currently leverages blueprints internally. Were we to move entirely to blueprints being defined in Superset, that would have substantial implications for the extent that we can leverage FAB in endpoints without either reworking FAB internals or adding the ability to nest blueprints to Flask. As we've been building out `/api/v1`, we've leveraged Flask's base classes, especially for GET endpoints. How would you recommend reconciling this problem? I think that @dpgaspar's suggestion for adding a `register_views` function to the `manager.py` files is a possible resolution - this would provide an API similar to blueprint functionality that could be leveraged in `app.py`. I'd be interested in your opinion on how close this would get us to resolving this issues you're bringing up. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization
willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization URL: https://github.com/apache/incubator-superset/issues/9077#issuecomment-586414197 First, thanks for your comments! I appreciate the community taking the issue seriously. @DiggidyDave I agree that empty `__init__.py` files will help substantially. @dpgaspar has a PR open right now that's linked to this issue that demonstrates the patterns we're suggesting. IMHO the result is very readable and easy to work with. We hope for this SIP to be a North Star that we gradually work towards. I like your suggestion of implementing valuable bits in chunks. I'd recommend being pragmatic and attacking small enough pieces that they are comprehensible. One of the areas I'm most excited to tackle is slimming down the model layer using DAOs and commands - I think models are far too thick at the moment, but I would treat this work alone as multiple coherent changes rather than one large PR due to the potential complexity involved. @john-bodley Superset currently has examples of both the Command pattern and a pattern similar to DAOs. Alembic's individual migration classes are an example of a command and FAB's shared filters are very similar to a DAO, in that they encapsulate shared query logic outside of the model. A DAO allows us to extract query logic from model classes to a clear destination. As we refactor, we need a target to move towards. Blueprints separate functionality well at the API layer, but will not provide a clean abstraction at the model or business logic layer without a few other patterns to lean on. I agree that fully refactoring the code is a large unit of work and expect this to be a target we move towards gradually where it makes sense. Currently the model layer is very thick and we have a lot of business logic in endpoint code. To me this is a difficult problem to solve without a shared concept of where this code should go moving forward and a pattern to leverage. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
[GitHub] [incubator-superset] willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization
willbarrett commented on issue #9077: [SIP-35] Proposal for Improving Superset’s Python Code Organization URL: https://github.com/apache/incubator-superset/issues/9077#issuecomment-584776316 @craig-rueda I've centralized the models in the recommendation, but have left the DAOs distributed - I don't think the DAOs will be too heavily reused outside of their domain. If the future proves me wrong, it can be refactored. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org For additional commands, e-mail: notifications-h...@superset.apache.org
