Make it possible to set WeasyPrint options #79
Conversation
|
Review changes with SemanticDiff. Analyzed 3 of 4 files. Overall, the semantic diff is 11% smaller than the GitHub diff.
|
django_weasyprint/views.py
Outdated
| @@ -82,6 +83,7 @@ def get_document(self): | |||
| return html.render( | |||
| stylesheets=self.get_css(base_url, url_fetcher, font_config), | |||
| font_config=font_config, | |||
| option=self._options, | |||
There was a problem hiding this comment.
Shouldn’t we pass options as separate keyword arguments? The API asks for **options, not option. Something like **self._options instead of option=self._options is probably appropriate.
There was a problem hiding this comment.
thank you for your comments! i think you accidentally pasted the wrong link, @liZe ;)
API: https://doc.courtbouillon.org/weasyprint/stable/api_reference.html#weasyprint.HTML.render
yes, if we want to pass the "options" all at once, the kwarg should definitely be called options. it is a dict of potentially multiple options.
however, that may be cause for confusion/bugs, as we already provide a stylesheets kwarg, which actually is a DEFAULT_OPTIONS parameter.
a good way around this may be using setdefault to add stylesheets to self._options before passing it to render() using **-unpacking. maybe a copy of the options dict is a good idea (similar to weasyprint's render()).
There was a problem hiding this comment.
thank you for your comments! i think you accidentally pasted the wrong link, @liZe ;)
🤣
django_weasyprint/views.py
Outdated
| @@ -90,7 +92,7 @@ def rendered_content(self): | |||
| Returns rendered PDF pages. | |||
| """ | |||
| document = self.get_document() | |||
| return document.write_pdf() | |||
| return document.write_pdf(options=self._options) | |||
There was a problem hiding this comment.
yes, should use **; no need for copy/setdefault, i think, since we do not provide any other kwargs here (valid kwargs other than options here are: target, zoom, finisher).
There was a problem hiding this comment.
Thank you for the suggestion and contribution, @dekkers ! ❤️
Please add a test, where the new pdf_options attribute of WeasyTemplateResponseMixin is used and make sure it still works, with pdf_stylesheets provided aswell.
An update to the example in the README would also be much apprechiated.
django_weasyprint/views.py
Outdated
| @@ -82,6 +83,7 @@ def get_document(self): | |||
| return html.render( | |||
| stylesheets=self.get_css(base_url, url_fetcher, font_config), | |||
| font_config=font_config, | |||
| option=self._options, | |||
There was a problem hiding this comment.
thank you for your comments! i think you accidentally pasted the wrong link, @liZe ;)
API: https://doc.courtbouillon.org/weasyprint/stable/api_reference.html#weasyprint.HTML.render
yes, if we want to pass the "options" all at once, the kwarg should definitely be called options. it is a dict of potentially multiple options.
however, that may be cause for confusion/bugs, as we already provide a stylesheets kwarg, which actually is a DEFAULT_OPTIONS parameter.
a good way around this may be using setdefault to add stylesheets to self._options before passing it to render() using **-unpacking. maybe a copy of the options dict is a good idea (similar to weasyprint's render()).
f6adcd7 to
cfeb259
Compare
|
Thanks for the feedback. Took me a while to find the time to properly fix this, but I've fixed the passing of the options and used |
|
This is brilliant!! An awesome fix |
WeasyPrints can be given an options dict in the
renderandwrite_pdfmethods. This PR makes it possible to do that with django-weasyprin by setting thepdf_optionsattribute on the view class or by implementing theget_pdf_optionsmethod.