-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
feat(viz): export csv with verbose_name #17341
Conversation
Use verbose_map to help the column names of csv export to be consistent with the front-end display
Codecov Report
@@ Coverage Diff @@
## master #17341 +/- ##
==========================================
- Coverage 77.19% 77.09% -0.10%
==========================================
Files 1036 1058 +22
Lines 55687 56482 +795
Branches 7627 7627
==========================================
+ Hits 42987 43545 +558
- Misses 12444 12681 +237
Partials 256 256
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Hi @chuancyzhang , code looks good to me, Could you fix the code style? Thanks for your contribution! |
OK |
What do you think about this change? @villebro |
@zhaoyongjie I personally agree with this change, but I wonder if this is considered a breaking change. Ping @etr2460 @john-bodley , any thoughts on this? |
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.
This request has come up multiple times in the past and I agree that this is probably expected behavior, but would prefer to get more opinions on this before hitting merge.
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.
I also approve of the change.
If there is a concern that this will change the current behavior and can be considered as a breaking change. We can add a checkbox in the control panel(disabled by default) to 'Use Labels in Export CSV'. This will allow users to get this feature without changing the current behavior, as we agree that this feature is needed for sure. |
@kamalkeshavani-aiinside this needs a rebase. I think we can merge this as-is, as adding yet another checkbox will just add clutter IMO. |
Hi @chuancyzhang , a friendly notice, this PR need to rebase master. Thanks! |
done |
There are conflicts as well. |
Please refer to PR #17657 .This PR, my rebase failed. @zhaoyongjie @villebro |
I think you don't need to a lot of changes to this PR. please double check. |
I thought the wrong way at the time, which caused patch-3 to fail to rebase, which resulted in so many changes. |
Yep, there are a couple of changes recently in the codebase. The If you have any questions, please ping me. |
I reopened a PR #17657 again, you can look at this |
Thanks! Do you mind I close this PR, then we can link new PR to this. |
no problem. thanks for you help. |
The column names displayed on the report are inconsistent with the column names of the actual exported csv file, which can sometimes cause confusion. Therefore, I hope to add a feature to make the column names displayed on the report consistent with the column names of the exported csv file.
SUMMARY
Use verbose_map to help the column names of csv export to be consistent with the front-end display.
Because of the revision of the Superset interface, this PR is changed. #10760
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION