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

add an option to export csv file with verbose column names #7795

Closed
wants to merge 1 commit into from

Conversation

@xcaptain
Copy link

xcaptain commented Jun 28, 2019

CATEGORY

Choose one

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

SUMMARY

In some cases we need to export csv file with custom table headers, this change enables to download csv with labels configured in datasource columns.

TEST PLAN

  1. in chart detail, edit datasource and add a label for each column
  2. click the 'export to .csv format' button to export csv
  3. the exported csv file will contain those label in first row
@pull-request-size pull-request-size bot added the size/XS label Jun 28, 2019
superset/viz.py Outdated Show resolved Hide resolved
superset/config.py Outdated Show resolved Hide resolved
@xcaptain xcaptain force-pushed the xcaptain:csv_verbose branch from 9d10805 to c6c803c Jul 9, 2019
@xcaptain

This comment has been minimized.

Copy link
Author

xcaptain commented Jul 9, 2019

@villebro Thank you for the review, I fixed the naming issue and rebased the pull request. this enhancement works well on my server, hope it can get merged, it's really useful for those none-tech people to export a meaningful CSV file.

@villebro

This comment has been minimized.

Copy link
Contributor

villebro commented Jul 9, 2019

@xcaptain you need to run black on the file you've edited as it is now a requirement on CI.

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jul 10, 2019

Codecov Report

Merging #7795 into master will decrease coverage by <.01%.
The diff coverage is 40%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7795      +/-   ##
==========================================
- Coverage   65.88%   65.88%   -0.01%     
==========================================
  Files         460      460              
  Lines       22028    22033       +5     
  Branches     2418     2418              
==========================================
+ Hits        14514    14516       +2     
- Misses       7394     7397       +3     
  Partials      120      120
Impacted Files Coverage Δ
superset/config.py 94.04% <100%> (+0.03%) ⬆️
superset/viz.py 71.66% <25%> (-0.12%) ⬇️

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 8c9b4b5...bf0b865. Read the comment docs.

Copy link
Contributor

villebro left a comment

Small change requested.

superset/config.py Outdated Show resolved Hide resolved
@xcaptain

This comment has been minimized.

Copy link
Author

xcaptain commented Jul 10, 2019

image
image

Added some images to show how this change work

Copy link
Contributor

villebro left a comment

LGTM. It would be neat to have a few unit tests for this (for both True and False case), but I think we can live without them if that feels difficult (I can add them later if needed).

@xcaptain

This comment has been minimized.

Copy link
Author

xcaptain commented Jul 10, 2019

Thanks, actually I don't know how to write unit tests for this commit, would be awesome if you could help writing tests. Do I need to rebase before it gets merged?

Copy link
Contributor

villebro left a comment

Sorry for picking up on these late, but a few additional questions/change requests. I can fill in unit tests later, so don't worry about those.

superset/viz.py Outdated Show resolved Hide resolved
superset/viz.py Outdated Show resolved Hide resolved
superset/viz.py Show resolved Hide resolved
Copy link
Contributor

villebro left a comment

LGTM

@mistercrunch

This comment has been minimized.

Copy link
Contributor

mistercrunch commented Jul 11, 2019

This seems like a setting that would apply to each export (as opposed to a global configuration setting...)

if set CSV_EXPORT_USE_VERBOSE_NAMES_AS_HEADERS to True then use verbose name
as csv file headers else use the default column name.
@xcaptain xcaptain force-pushed the xcaptain:csv_verbose branch from db82186 to 6fa938f Jul 11, 2019
@xcaptain

This comment has been minimized.

Copy link
Author

xcaptain commented Jul 11, 2019

@villebro I rebased my commits, it's now ready to be merged. @mistercrunch it's useful to use verbose names as csv headers because usually only non-tech people read those csv files. but json files are usually used by programmer, so I think it's enough to only add this option for csv exporting.

@villebro

This comment has been minimized.

Copy link
Contributor

villebro commented Jul 11, 2019

@mistercrunch were you proposing adding a sub-menu under the export button or something similar?

@xcaptain

This comment has been minimized.

Copy link
Author

xcaptain commented Jul 18, 2019

ping @villebro , can this commit get merged?

@mistercrunch

This comment has been minimized.

Copy link
Contributor

mistercrunch commented Jul 18, 2019

Yes I meant something like that. It's unclear what people may want, or even whether they care, and a global config setting doesn't seem like the right way to go about it.

Either we always prefer verbose names, or give the option to users on export. The option itself may be confusing to most users...

@villebro

This comment has been minimized.

Copy link
Contributor

villebro commented Jul 18, 2019

In that case I vote always using verbose names. Quickly toyed with the idea of having a submenu, but that felt overkill and confusing to users who just want to get the data out.

@stale

This comment has been minimized.

Copy link

stale bot commented Sep 16, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive label Sep 16, 2019
@xcaptain xcaptain closed this Sep 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.