-
-
Notifications
You must be signed in to change notification settings - Fork 143
Conversation
…are now passing when run via npm test
@emilhe Thanks so much for this PR, and my sincere apologies for not reviewing it earlier. Looks very promising! A few general thoughts before I get into specific code comments:
|
dash_core_components/express.py
Outdated
"to_parquet": True, | ||
"to_msgpack": True, | ||
"to_stata": True, | ||
"to_pickle": True, |
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 True
/False
looks funny - as if False
means "not a known writer"
Perhaps the values could just be send_bytes
/send_string
?
That said, are the functions send_bytes
and send_string
useful on their own? With writer
as the first arg it feels pretty specific to pandas, and it doesn't look like a helper (or b64 encoding) is necessary for strings anyway - you just use dict(content=the_string, filename=the_filename)
right? Does the same apply to byte strings? If that's right and these are only useful internally I'd give them underscored names like _send_pd_string
- or even inline them in send_data_frame
and call this dict _pandas_writes_bytes
- that would be DRYer since the only differences between the two functions are BytesIO
vs StringIO
and whether you .encode()
after .getvalue()
.
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.
Ah, yes, i guess that would actually make more sense :)
Yes, I consider the send_bytes
and send_string
functions to be usefull on their own. The writer can be any object, not just pandas writers. The most common use case is probably sending excel files, but it could be anything. It's not more than a few weeks ago since i got the latest request on how to send a particular file type, in this case pdf files created by pdfrw
. Since pdfrw
creates binary files, the solution is
send_bytes(lambda buf: pdfrw.PdfWriter().write(buf, my_pdf_data), filename="my_pdf.pdf")
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.
Can confirm send_bytes is useful (I was the one who asked about pdfrw). It's working great within my dash app currently
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.
OK, that makes sense. My main concern was that you have to pass a function in, and I imagine in a lot of these situations you'll already have a string or bytestring so it would feel funny to have to wrap that in a lambda
. What if we allowed that first parameter to be either a writer function or a string / bytestring? For character strings that's only a slight simplification vs constructing the dict yourself, but still may be easier for users to learn. For bytestrings the argument feels more solid, as send_bytes
ensures we get the encoding right.
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 see you point. That being said, i haven't yet encountered a case where I have a bytes
object, passing around a writer seems to be more common (at least in my experience). I have now refactored the code and made some minor changes so that,
send_bytes
now accepts abytes
object or a writer compatibale withBytesIO
send_string
now accepts astr
object or a writer compatiable withStringIO
While send_string
doesn't simplify much as compared to constructing the dictionary manually, i actually like that there are util methods for all common use cases. This way, the data structure (a dict with certain properties) is hidden from the user; it essentially becomes an implementation detail.
Co-authored-by: Alex Johnson <johnson.alex.c@gmail.com>
* Reverting test files from black 20.8b1 to 19.10b0 (for compatibility) * Moving express.py to dash_core_components_base + added import in __import__ (+ updated tests accordingly)
|
…/bytes in addition to writers.
@emilhe Sorry about the long delay here - I think I've fixed a couple of remaining minor issues, could you give me (ie maintainers) edit permissions on this PR to push my changes? |
Closing in favor of #926 (which is just the same branch plus some updates) |
As per the discussion #216, this pull request is an attempt to merge my unofficial
Download
component (currently residing in thedash-extensions
library) intodash-core-components
. The simplest use case is download of raw data,In addition to the
Download
component itself, a few utility functions are included that makes it easy to download files,and data frames,
I was not really sure where to put these util functions, so for now i have put them in a new
express
module in dash_core_components. I guess there are (at least) a few points to discuss,I have added tests for all three cases (raw content, file, data frame), but i haven't added an example in the Demo, as i haven't been able to get it running (see #805 ).
EDIT: I am also having some issues with lint; running black locally, my tests .py files are OK. But on the build server they are only OK for python 2.7, not for python 3.6 and 3.7 (locally i am running 3.7).