-
Notifications
You must be signed in to change notification settings - Fork 363
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pdf export capability using xhtml2pdf #249
Conversation
Added a test which passes. The failing travis-ci is because of djcelery, I'm not sure why. I have commented out djcelery references from |
Celery just got upgraded to version 4.0, which moved the |
@grantmcconnaughey cool! However, since this is not really related to the PR I propose to push this change (pinning djcelery version) to master through a different PR and I will then re-merge master to my pdf-export branch. Thanks! |
@spapas pinned! Feel free rebase from master |
It seems that django-xhtml2pdf does not support python 3, as per xhtml2pdf/xhtml2pdf#190 How could we proceed with this ? |
I'm OK with the idea of feature-detecting py2 vs. py3 and only executing the tests, and including the exporter, if it's py2. Please just update the readme to reflect that. Sound like the right approach to you? |
@chrisclark yes that's great thanks! I'll update the PR soon. |
and settings for that
]) | ||
if sys.version_info[0] < 3: |
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.
Since this stuff is in the optional requirements, I think we should do a try...except around importing one of the dependencies as well as checking the Python version before importing it. Otherwise we leave open the possibility of runtime errors when folks select the PDF download and the deps aren't installed.
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.
Hello @chrisclark , I've added it in a try catch with the correct import - if python version > 2 or django-xhtml2pdf is not installed pdf exported will not be included.
|
||
|
||
@unittest.skipIf(sys.version_info[0] > 2, "not supported in this library version") |
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 think the move is to probably add a utility function like
def pdf_supported():
# try to import the dependency
# and python version == 2
return True else False
Then use that both here and in app_settings.py
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 don't really like adding a utility function (in utils.py) because that would introduce a circular import. For the testing if pdf exporting we can just check if python version < 2 (the optional requirements should be installed anyway for testing). So I propose leaving it like this (I've just improved the message a bit).
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.
That makes sense, thanks.
Took at a look at the PR. It's generally fine...but it gives me some anxiety that xhtml2pdf is an abandoned project, and the django integration is 4+ years old without any maintenance. The xhtml2pdf project recommends using weasyprint, which looks incredibly simple, has fewer dependencies, and supports Python3. The exporter would just be something like:
If you're up for doing it WeasyPrint, I'll merge in a heartbeat. If not, I get it, and I would reluctantly accept this PR since it's clear people need it, but I'd open an issue to replace with WeasyPrint as soon as someone can find the time. Would love to hear feedback from others as well. |
Hello @chrisclark, first of all, xhtml2pdf is not an abandoned project, there is an effort of adding python 3 support to it. Also, the django-xhtml2pdf is just a single view with 10 lines (integrate django templates with xhtml2pdf) that's why it doesn't actually need any updates. Both xhtml2pdf and django-xhtml2pdf work fine with all my (django 1.10) projects. Now, for weasyprint. I am aware of that project but I don't use it since it is really difficult (if possible at allo) installing it on windows and I am using windows as my dev machine. Please take a look at its requirements http://weasyprint.readthedocs.io/en/latest/install.html#installing, and more importantly its windows installation: http://weasyprint.readthedocs.io/en/latest/install.html#windows. I refuse to do all this stuff when, using xhtml2pdf is one So I can't go the weasyprint way :( What I recommend is merge my PR but, if somebody wants to implement support for WeasyPrint pdf export, implement this in addition to xhtml2pdf pdf export by adding another Exported (for example WeasyPrintPdfExporter) and then, in app_settings.py check if weasyprint is installed to use the WeasyPrintPdfExporter, then check if django-xhtml2pdf is installed to use PdfExporter. That's what I'm going to do at least, I can't use WeasyPrint until it's easy to install on windows and xhtml2pdf works really good. |
Hah, yep -- that is definitely not a fun install experience. Just to clarify though; it is absolutely an abandoned project. From the home page:
But like I said, Im ok merging since it's a solid solution. Thanks for all the work on it! |
Ooh I didn't know that. Really sorry xhtml2pdf is dead ... It served me (and still serves me actually) well in a lot of projects. Thanks for merging though, this will be very useful 👍 |
Hello, just a heads up - xhtml2pdf just got a new maintainer (xhtml2pdf/xhtml2pdf#317) and was resurrected from the dead! So you no longer have to feel guilty for merging this PR 😎 |
Phew! I have't slept in 2 weeks! And seriously - thanks for the update. This does actually reduce my anxiety. |
Add an extra exporter (PDF) using xhtml2pdf and reportlab. Uses the
exporer/pdf_template.html
as a template for the pdf table, you can override it in your own apps to add (for example) page numbers, extra fonts, change page layout etc. Should fix #207.