You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/01/02 18:54:33 UTC
[GitHub] [incubator-superset] craig-rueda opened a new pull request #8912:
Moving appbuilder.xxx out of view files and into app.py
craig-rueda opened a new pull request #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912
### CATEGORY
Choose one
- [ ] Bug Fix
- [ ] Enhancement (new features, refinement)
- [x] Refactor
- [ ] Add tests
- [ ] Build / Development Environment
- [ ] Documentation
### SUMMARY
This is a follow-up to #8418
* Moves `appbuilder.add_view` / `appbuilder.add_link` / etc. from view files into `app.py`
* Updates the docker_init.sh message a bit
### REVIEWERS
@dpgaspar @mistercrunch
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar commented on a change in pull request
#8912: Moving appbuilder.xxx out of view files and into app.py
Posted by GitBox <gi...@apache.org>.
dpgaspar commented on a change in pull request #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912#discussion_r362589463
##########
File path: superset/app.py
##########
@@ -114,10 +116,318 @@ def __call__(self, *args, **kwargs):
celery_app.Task = AppContextTask
- @staticmethod
- def init_views() -> None:
- # TODO - This should iterate over all views and register them with FAB...
- from superset import views # noqa pylint: disable=unused-variable
+ def init_views(self) -> None:
+ #
+ # We're doing local imports, as several of them import models which in turn try to import
+ # the global Flask app
+ #
+ from superset.connectors.druid.views import (
Review comment:
I would prefer if possible, to split this. For example on dashboards create a `DashboardManager` that would implement an init_views, then main `app.py` would invoke `init_views` for each manager.
So each package would be responsible for orderly adding what it implements.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] craig-rueda commented on a change in pull
request #8912: Moving appbuilder.xxx out of view files and into app.py
Posted by GitBox <gi...@apache.org>.
craig-rueda commented on a change in pull request #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912#discussion_r362591283
##########
File path: superset/app.py
##########
@@ -114,10 +116,318 @@ def __call__(self, *args, **kwargs):
celery_app.Task = AppContextTask
- @staticmethod
- def init_views() -> None:
- # TODO - This should iterate over all views and register them with FAB...
- from superset import views # noqa pylint: disable=unused-variable
+ def init_views(self) -> None:
+ #
+ # We're doing local imports, as several of them import models which in turn try to import
+ # the global Flask app
+ #
+ from superset.connectors.druid.views import (
Review comment:
Absolutely makes sense. I would like to do that as another step, however. This change will let us move in that direction as it removes some of the side effects that exist in the current setup. At some point, I'll pull relevant pieces out into their own "configuration" classes which will do just what you said - break things up and allow for a bit more composability. I'd also like to update the CLI WRT these changes.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io edited a comment on issue #8912:
Moving appbuilder.xxx out of view files and into app.py
Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on issue #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912#issuecomment-570341513
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=h1) Report
> Merging [#8912](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/28563ad088b237b8d15ec801925a2d3cdca5bcfd?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8912 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=footer). Last update [28563ad...6b6b599](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] mistercrunch commented on a change in pull
request #8912: Moving appbuilder.xxx out of view files and into app.py
Posted by GitBox <gi...@apache.org>.
mistercrunch commented on a change in pull request #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912#discussion_r362879459
##########
File path: superset/app.py
##########
@@ -114,10 +116,318 @@ def __call__(self, *args, **kwargs):
celery_app.Task = AppContextTask
- @staticmethod
- def init_views() -> None:
- # TODO - This should iterate over all views and register them with FAB...
- from superset import views # noqa pylint: disable=unused-variable
+ def init_views(self) -> None:
+ #
+ # We're doing local imports, as several of them import models which in turn try to import
+ # the global Flask app
+ #
+ from superset.connectors.druid.views import (
Review comment:
There will be a fair amount of view shuffling as we break down and reorg modules. First goal is to move view init out of module scope, then find the right home for each view and init call. This is a good step in the right direction.
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] dpgaspar merged pull request #8912: Moving
appbuilder.xxx out of view files and into app.py
Posted by GitBox <gi...@apache.org>.
dpgaspar merged pull request #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
[GitHub] [incubator-superset] codecov-io commented on issue #8912: Moving
appbuilder.xxx out of view files and into app.py
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8912: Moving appbuilder.xxx out of view files and into app.py
URL: https://github.com/apache/incubator-superset/pull/8912#issuecomment-570341513
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=h1) Report
> Merging [#8912](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/4b95c1f51719593fcc3ddcbba7effec11968945d?src=pr&el=desc) will **not change** coverage.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8912 +/- ##
=======================================
Coverage 58.97% 58.97%
=======================================
Files 359 359
Lines 11333 11333
Branches 2787 2787
=======================================
Hits 6684 6684
Misses 4471 4471
Partials 178 178
```
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=footer). Last update [4b95c1f...be972dc](https://codecov.io/gh/apache/incubator-superset/pull/8912?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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:
users@infra.apache.org
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org