-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
test: rename column names for birth_names dataset #12082
Conversation
thanks for making the change. |
Codecov Report
@@ Coverage Diff @@
## master #12082 +/- ##
==========================================
- Coverage 66.36% 62.81% -3.56%
==========================================
Files 1014 1014
Lines 49513 49499 -14
Branches 5066 5066
==========================================
- Hits 32861 31094 -1767
- Misses 16508 18196 +1688
- Partials 144 209 +65
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
952da23
to
80bd4d5
Compare
@@ -63,7 +63,7 @@ def gen_filter( | |||
|
|||
|
|||
def load_data(tbl_name: str, database: Database, sample: bool = False) -> None: | |||
pdf = pd.read_json(get_example_data("birth_names.json.gz")) | |||
pdf = pd.read_json(get_example_data("birth_names2.json.gz")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dataset was updated in apache-superset/examples-data#7
ac3cbf6
to
34e29a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fyi: @adam-stasiak you may wanna update test cases accordingly
Looks like this introduced a regression with example dashboards. @junlincc let's ensure PRs are code reviewed before being approved. |
I wouldn't categorize this as a regression, at most an inconvenience for developers. I personally would nuke examples database all the time whenever a new example dashboard is added. We were aware of the risk of breaking the example dashboard, but decided to take the risk. In addition to much more work, it would also be a little weird to write a db migration or anything special just for this when most production deployments wouldn't load the examples to begin with. Maybe better documentation next time. |
@robdiciuccio i don't consider this as a regression neither, it's an improvement. As I added in the comment, those column names are not clear which mislead users to read the chart. And i actually did the code review as well, I understood all the changes in 11 files. In terms of the risk of breaking dashboard, I specifically asked Adam to update the test case accordingly. |
Definitely seems worthy of a mention in UPDATING.md at the very least, as use cases for example dashboards are clearly different for different organizations. |
sure. |
SUMMARY
Rename
sum_girls
tonum_girls
andsum_boys
tonum_boys
in the examplebirth_names
dataset as they are kind of confusing when used in combination with adhoc metrics.Must wait for: apache-superset/examples-data#7
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A
TEST PLAN
CI and local visual verification
ADDITIONAL INFORMATION