Skip to content
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

Added option to ignore ContentNotFoundError #128

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Added option to ignore ContentNotFoundError #128

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 27, 2017

My use case involved rendering lots of templates with user-uploaded images so bombing on a 404 was a deal breaker. Investigated other options, but wkhtmltopdf seemed like the best fit so...

A quick glance at the wkhtmltopdf source didn't reveal any way to do this with return codes, so I resorted to checking stderr. Had to rename subprocess.py since I'm no longer using the native implementation of check_source().

Added a few doctests to process.py

To use the new behaviour, set:

WKHTMLTOPDF_ENV = {
    'ignore_404': 'True'
}

In your settings.py.

@johnraz
Copy link
Collaborator

johnraz commented Jan 28, 2017

@Filipp do you mind splitting this f857631 in 2 different commits please, one that renames the file and another that adds your changes. Will make the review easier on me ;-) Thanks !

@johnraz
Copy link
Collaborator

johnraz commented Jan 28, 2017

Also as it seems to be related to the long standing issue about raising CalledProcessError in an improper way (see issues: #122 #112 #107), and as you have already put some effort around it, it would be really nice to extend this PR to match the actual behavior of wkhtml2pdf and also improve the output in case of errors.
Again, this is only a proposal but that would greatly improve the lib, IMHO ;-)

@ghost
Copy link
Author

ghost commented Jan 29, 2017

I honestly don't know how to split the commits to pre/post file rename. I think we should actually just move check_output() to utils.py. :)

If someone could point me to the relevant wkhtmltopdf documentation or source on its different exit codes then I would be more than happy to add the proper handling. We should probably create exceptions for the different error scenarios withe corresponding options to ignore them in one's Django app.

@johnraz
Copy link
Collaborator

johnraz commented Jan 29, 2017

You just need to go back in your git history with git rebase -i f857631^, edit the commit, move the file, commit, do your changes commit again. The problem would be the same with moving the code to utils.py btw, you'd have to first move the code without changing it, commit, do the changes, commit again.

I looked quickly and there doesn't seem to be a proper documentation... wkhtmltopdf...
And from what I've read here, it seems to be difficult to really predict everything...

I guess we can start with what you did here and maybe at least improve error logging to give better feedback when something goes wrong.

Thanks for your time anyway.

@johnraz
Copy link
Collaborator

johnraz commented Mar 5, 2017

@Filipp do you think you could maybe split that commit up and I'll be very happy to merge this :-)

@ashishnitinpatil
Copy link

ashishnitinpatil commented Oct 7, 2017

I got a TypeError: a bytes-like object is required, not 'str' in line 73 of process.py if 'ContentNotFoundError' in error_message: on Python3.5

Changing to if 'ContentNotFoundError' in str(error_message): solved the issue. Didn't look deeper into it.

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