File objects for publish workbook #704
Conversation
|
@t8y8 @shinchris How do you feel about the concept/implementation? |
|
I'm not opposed to the idea, but I need to take a deeper look. Do I understand the primary use case as being "I need to hold a workbook xml in memory, possibly modifying it, and then eventually publish it"? |
|
Yes that's about it. Modified xml could then be published either packaged or non-packaged without writing it to the filesystem. Thanks for looking into this! |
There was a problem hiding this comment.
Great idea — I understand the use case and think it’s worth pursuing.
I left some specific comments, mostly a small nit.
I do have a larger concern about how we would sprinkle a bunch of if path checks in the already somewhat if statement heavy code. I’m wondering if there’s a way to avoid that?
| if is_file_path: | ||
| file_size = os.path.getsize(file) | ||
| else: | ||
| file.seek(0, os.SEEK_END) |
There was a problem hiding this comment.
I had to look this up, but I see what’s going on here — is it possible to safely use os.fstat(fd).st_size? If not, can you extract this into a little utility function named get_file_size or something similar?
There was a problem hiding this comment.
As I understood it, in memory binary streams such as BytesIO() don't have a file descriptor that is needed for os.fstat. I'll extract the logic to a utility function
from io import BytesIO()
import os
with open('file.txt', 'wb') as f:
f.write('Hello World!'.encode())
f1 = open('file.txt')
f2 = BytesIO()
f2.write('Hello World!'.encode())
print('f1 size', os.fstat(f1.fileno()).st_size)
print('f2 size', os.fstat(f2.fileno()).st_size)
> f1 size 12
> Traceback (most recent call last):
> File "<stdin>", line 1, in <module>
> io.UnsupportedOperation: fileno
|
@nnevalainen Alright, a quick review! I need to ponder a bit on the if checks question, but overall it feels close/the right direction. |
Should we try to get rid of the "if path" checks altogether and adopt duck typing approach? We could group all operations using file path together like |
Yes, I like that approach, if we need to add another method in the future, or update something, we can keep it contained to the try block. Ship it! |
|
Other than the requested changes from @t8y8, this looks good to me. |
|
Yeah, I think the use case is still valid for TDS/TDSX, and maybe even flows (TFL/TFLX). @nnevalainen would you be interested in doing that as well? |
|
I can take a look at that as well in a separate PR after finishing this one |
… into feature/file-objects-for-publish-workbook
|
Alright finished with the the changes. Let me know what you think |
|
@nnevalainen forgot to ask earlier, but would you be able to read and sign our CLA? |
This is a proposal for an enhancement for workbook.publish to support a file object parameter.
User story
As a TSC user I want to publish workbooks by providing a file object, so that I don't have to store temporary files to a file system
Specific use case
Acceptance criteria