villebro commented on a change in pull request #9825:
URL:
https://github.com/apache/incubator-superset/pull/9825#discussion_r431590674
##########
File path: superset/app.py
##########
@@ -342,6 +347,15 @@ def init_views(self) -> None:
category_label=__("Sources"),
category_icon="fa-wrench",
)
+ appbuilder.add_link(
+ "Upload Excel",
+ label=__("Upload Excel"),
+ href="/exceltodatabaseview/form",
+ icon="fa-upload",
+ category="Sources",
+ category_label=__("Sources"),
+ category_icon="fa-wrench",
+ )
Review comment:
It would be great if this were conditionally added if any of the
`EXCEL_EXTENSION` are present in `ALLOWED_EXTENSIONS`. Something like
```python
if EXCEL_EXTENSION.intersection(ALLOWED_EXTENSIONS):
appbuilder.add_link(...)
```
Same for CSV.
##########
File path: setup.py
##########
@@ -109,6 +109,7 @@ def get_git_sha():
"sqlalchemy-utils>=0.33.2,<0.36.5",
"sqlparse>=0.3.0, <0.4",
"wtforms-json",
+ "xlrd>=1.2.0",
Review comment:
I assume this dependency is required by Pandas for uploading Excel
files? I checked the Github page, and apparently the library is no longer
actively maintained:
> PLEASE NOTE: This library currently has no active maintainers. You are
advised to use OpenPyXL instead. If you absolutely have to read .xls files,
then xlrd will probably still work for you, but please do not submit issues
complaining that this library will not read your corrupted or non-standard
file. Just because Excel or some other piece of software opens your file does
not mean it is a valid xls file.
https://github.com/python-excel/xlrd
I wonder if we could replace `xlrd` with
[OpenPyXL](https://openpyxl.readthedocs.io/en/stable/), which seems to be
actively maintained?
##########
File path: superset/config.py
##########
@@ -351,8 +351,9 @@ def _try_json_readsha(filepath, length): # pylint:
disable=unused-argument
SUPERSET_WEBSERVER_DOMAINS = None
# Allowed format types for upload on Database view
-# TODO: Add processing of other spreadsheet formats (xls, xlsx etc)
-ALLOWED_EXTENSIONS = {"csv", "tsv"}
+EXCEL_EXTENSION = {"xlsx", "xls"}
+CSV_EXTENSION = {"csv", "tsv"}
Review comment:
Nit: perhaps call these in plural, i.e. `EXCEL_EXTENSIONS` and
`CSV_EXTENSIONS`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
