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

Moving appbuilder.xxx out of view files and into app.py #8912

Merged
merged 6 commits into from
Jan 8, 2020

Conversation

craig-rueda
Copy link
Member

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • 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

# 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 (
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

@codecov-io
Copy link

codecov-io commented Jan 2, 2020

Codecov Report

Merging #8912 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28563ad...6b6b599. Read the comment docs.

# Conflicts:
#	superset/views/dashboard/api.py
#	superset/views/log/api.py
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

LGTM

@dpgaspar dpgaspar merged commit 2a94150 into apache:master Jan 8, 2020
@dpgaspar dpgaspar deleted the fab_config branch January 8, 2020 18:04
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.36.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/XL 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants