Skip to content

Conversation

@nnevalainen
Copy link
Contributor

Extend support for publishing file objects to datasource.publish as discussed in #704. I followed the same approach as with workbooks. Let me know what you think :)

error = "Datasource item must have a name when passing a file object"
raise ValueError(error)

file_extension = 'tdsx' if file_is_compressed(file) else 'tds'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might have a problem here -- you can directly publish a tde or hyper file, both of which are actually compressed file formats themselves (I'm not sure if they're both zip or some other format)

Copy link
Collaborator

@t8y8 t8y8 Nov 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be an issue practically -- if they're loading a tde or hyper file in memory it might just fail on publish -- and given that they can't really update it in memory, there's little reason to do this.

I wonder if there is a way to warn the caller early though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I modified the file_peeking to return file_type instead of boolean value and extended it to identify .xml, .tde, and .hyper. I couldn't find official sources for .tde or .hyper file signatures but was able to deduce them by creating few different files and peeking their content.

Now if users provides '.tde' or '.hyper' file they are informed that these are not-supported file types. Additionally, they will be shown an error if they're providing file types we can't identify. Let me know what you think

Copy link
Collaborator

@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small nits but approved.

The tests are excellent, thank you!

@nnevalainen
Copy link
Contributor Author

Made the changes pointed out. Should really learn to use pycodestyle locally. I've basically run into broken travis builds every time I've pushed something out 😅

It could be nice to force some local testing in the form of pre-push hooks or similar.

Copy link
Collaborator

@t8y8 t8y8 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@t8y8 t8y8 merged commit ab80907 into tableau:development Nov 5, 2020
This was referenced Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants