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

[dashboard] use filter_scopes metadata when import old dashboard #9145

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Feb 14, 2020

CATEGORY

Choose one

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

Update dashboard import action: convert old filter_immune_slices and filter_immune_slice_fields to use filter_scopes

TEST PLAN

CI and unit test

ADDITIONAL INFORMATION

REVIEWERS

@john-bodley @serenajiang @mistercrunch @etr2460

@graceguo-supercat graceguo-supercat changed the title [dashboard] update filter_scopes metadata when import old dashboard [dashboard] use filter_scopes metadata when import old dashboard Feb 14, 2020
@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9145   +/-   ##
=======================================
  Coverage   59.06%   59.06%           
=======================================
  Files         372      372           
  Lines       11922    11922           
  Branches     2919     2919           
=======================================
  Hits         7042     7042           
  Misses       4698     4698           
  Partials      182      182

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 f4ad15e...2ccc8dc. Read the comment docs.

Copy link
Contributor

@serenajiang serenajiang left a comment

Choose a reason for hiding this comment

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

I trust u. Just a few questions.

@@ -278,6 +278,12 @@ def alter_params(self, **kwargs):
d.update(kwargs)
self.params = json.dumps(d)

def remove_params(self, param, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we pass in **kwargs to this function anywhere? If not, can remove the kwargs parts.

Copy link
Contributor

Choose a reason for hiding this comment

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

also, typing plz ❤️

@@ -334,6 +323,29 @@ def alter_positions(dashboard, old_to_new_slc_id_dict):
old_slc_id_str
]

# since PR #9109, filter_immune_slices and filter_immune_slice_fields
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any way we can avoid having this special logic? I'm worried about code in Superset getting (even more) bloated. When will create_from_import have the extra fields? Will it always? Is it ok to just not support this case and warn that your change is not backwards compatible?

Copy link
Author

Choose a reason for hiding this comment

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

export dashboard and import dashboard is a widely used feature in Superset. it helps ppl to move dashboard from different data system (and both of them use Superset). If someone wants to copy a dashboard from an older system which didn't upgrade recently, but the other system used updated version, this PR can make their export - import routine keep working.

if the backward compatible only introduce a couple of lines, I would like to keep it. But if it will introduce a lot of overhead, i will not maintain it.

@@ -278,6 +278,12 @@ def alter_params(self, **kwargs):
d.update(kwargs)
self.params = json.dumps(d)

def remove_params(self, param_to_remove: str, **kwargs: Dict) -> None:
Copy link
Author

Choose a reason for hiding this comment

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

add typing. is it looks better?

def remove_params(self, param, **kwargs):
d = self.params_dict
d.pop(param, None)
d.update(kwargs)
Copy link
Author

Choose a reason for hiding this comment

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

used kwargs here.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, what I meant was - is this function ever called with keyword arguments? It looks like you only called it with the positional argument param_to_remove, so I don't thiink we need the kwargs logic.

Copy link
Author

Choose a reason for hiding this comment

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

fixed. Thanks!

@graceguo-supercat graceguo-supercat merged commit 84b42d2 into apache:master Feb 19, 2020
@graceguo-supercat graceguo-supercat deleted the gg-RemoveOldFilterImmuneMetadata branch June 11, 2020 23:20
@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/M 🚢 0.36.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants