Skip to content

Add support for PDF and CSV Export#236

Merged
t8y8 merged 7 commits intotableau:developmentfrom
t8y8:234-feature-new-exports
Sep 27, 2017
Merged

Add support for PDF and CSV Export#236
t8y8 merged 7 commits intotableau:developmentfrom
t8y8:234-feature-new-exports

Conversation

@t8y8
Copy link
Collaborator

@t8y8 t8y8 commented Sep 27, 2017

TODO:

  • Add all the options for PDF export (landscape, size)
  • Add tests for the new endpoints
  • Should PDF also return an iterable-stream?
  • How do we feel about csv returning an iterable-stream?
  • How do we feel about calling 'data' > 'csv' on the model?
  • Clean up error strings (They're find/replace right now)
  • Do we want to rename image to png?
  • I called 'type' for the pdf size 'output_size' but open to better names. I don't like type

/cc @RussTheAerialist

Copy link
Contributor

@LGraber LGraber left a comment

Choose a reason for hiding this comment

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

Generally, I think this points out some flaws in our current "populate" model because it basically breaks the idea that a model is just a property bag. By storing the function and executing a call to the server, we have kind of broken the implied contract. Having said that, it appears we have done that elsewhere now. I think we will need to take a look at this in the not so distant future to get this back to a clean and well expressed state ... even if that means we separate the low level entities from the objects which can be used to logically group them

@t8y8
Copy link
Collaborator Author

t8y8 commented Sep 27, 2017

Thanks!

Yeah, I agree we need to revisit the model-call-sync problems (tags still feel weird to me).
This implementation was the 'least bad' Russell and I could think of to solve paging lists of sub-models (like users in groups).

This PR has more work, so stay tuned for the updates.

PS, I can't wait for options-builder, these PDF options are long

@graysonarts
Copy link
Contributor

RE: data vs. csv - At some point we want to support json output. Would you see that as being a "json" endpoint also?

RE: type/outputSize - PageSize would fit the domain of pdf better than outputSize.

RE: Iterable Stream - for csv it makes sense(ish), but does pdf really make sense to be iterable? What's the use case? pdfs will generally be small enough since we are only doing one sheet/dashboard at a time.

@t8y8
Copy link
Collaborator Author

t8y8 commented Sep 27, 2017

re: json -- yeah, I think I slightly prefer that to something like populate_data(type=json). But I don't feel very strongly.

re: pageSize -> But we use that for the regular request options. I don't like it meaning 1-1000 on one and 'Letter' on another :P Any other ideas?

re: iterables -> It could technically be a 10000 by 100000 pixel dashboard with 1 billion marks. If we're not worried about that I can keep it simple and leave it as is. I agree it's very much an edge case and just paranoia.

@@ -0,0 +1,5 @@
<?xml version='1.0' encoding='UTF-8'?>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

er, this was to cover your other item but I mixed my CLs. Ignore for now.

@t8y8 t8y8 changed the title [WIP] Add support for PDF and CSV Exports Add support for PDF and CSV Export Sep 27, 2017
Copy link
Contributor

@graysonarts graysonarts left a comment

Choose a reason for hiding this comment

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

🚀

raise UnpopulatedPropertyError(error)
return self._pdf()

@property
Copy link
Contributor

Choose a reason for hiding this comment

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

Man, this pattern is screaming for a decorator or something.

@t8y8 t8y8 merged commit 728643e into tableau:development Sep 27, 2017
@t8y8 t8y8 deleted the 234-feature-new-exports branch September 27, 2017 22:28
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.

3 participants