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

convert position to v2 for Superset load_examples #5515

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Jul 28, 2018

update position_json for dashboards from load_examples.
this PR is based on #5543 and #5463.

@mistercrunch @williaster @michellethomas @john-bodley

@@ -351,7 +366,7 @@ def load_world_bank_health_n_pop():
secondary_metric='sum__SP_POP_TOTL',
series="country_name",)),
]
misc_dash_slices.append(slices[-1].slice_name)
# misc_dash_slices.append(slices[-1].slice_name)
Copy link
Member

Choose a reason for hiding this comment

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

leftover commented line

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

def update_slice_ids(layout_dict, slices):
charts = list(
filter(
lambda component: (
Copy link
Member

Choose a reason for hiding this comment

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

[optional] the more pythonic way of writing this is by using a list comprehension, something like

charts = [
    o for o in layout_dict.values()
    if isinstance(component, dict) and component['type'] == 'DASHBOARD_CHART_TYPE'
]

Copy link
Author

Choose a reason for hiding this comment

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

fixed.

@mistercrunch
Copy link
Member

Minor comments, otherwise LGTM

@graceguo-supercat graceguo-supercat force-pushed the gg-UpdateLayoutDataForSupersetExamples branch 2 times, most recently from db115d7 to 0b5d767 Compare July 31, 2018 23:18
@@ -33,7 +33,17 @@

DATA_FOLDER = os.path.join(config.get("BASE_DIR"), 'data')

misc_dash_slices = [] # slices assembled in a "Misc Chart" dashboard
misc_dash_slices = set() # slices assembled in a "Misc Chart" dashboard
Copy link
Author

Choose a reason for hiding this comment

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

in load_multi_line, some slices are loaded twice. I have to use Set to store misc_dash's slices name, otherwise same slices are added into list twice.

Copy link
Member

Choose a reason for hiding this comment

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

Are we worried that the order wont be deterministic?

Copy link
Author

Choose a reason for hiding this comment

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

misc_dash used charts that are generated by other load functions. I guess the order is not quite important.

@@ -183,6 +180,9 @@ def load_examples_run(load_test_data):
print('Loading [Multi Line]')
data.load_multi_line()

print('Loading [Misc Charts] dashboard')
Copy link
Author

Choose a reason for hiding this comment

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

load_multi_line also added slice into misc_dashboard. i switch the for these 2 load functions, so that generate misc_dashboards after all slices are ready.

@codecov-io
Copy link

codecov-io commented Jul 31, 2018

Codecov Report

Merging #5515 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5515      +/-   ##
==========================================
+ Coverage   63.25%   63.25%   +<.01%     
==========================================
  Files         351      351              
  Lines       22258    22261       +3     
  Branches     2470     2470              
==========================================
+ Hits        14079    14082       +3     
  Misses       8164     8164              
  Partials       15       15
Impacted Files Coverage Δ
superset/data/__init__.py 100% <100%> (ø) ⬆️
superset/cli.py 46.59% <100%> (ø) ⬆️

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 51bd17d...4981fda. Read the comment docs.

@graceguo-supercat graceguo-supercat force-pushed the gg-UpdateLayoutDataForSupersetExamples branch from 0b5d767 to 4981fda Compare August 3, 2018 21:23
@mistercrunch mistercrunch merged commit faf35b0 into apache:master Aug 3, 2018
@mistercrunch mistercrunch deleted the gg-UpdateLayoutDataForSupersetExamples branch August 3, 2018 22:28
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 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 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants