Skip to content

Allow asymmetric PDF export resolution#3764

Closed
tzjtan wants to merge 2 commits intopython-pillow:masterfrom
tzjtan:master
Closed

Allow asymmetric PDF export resolution#3764
tzjtan wants to merge 2 commits intopython-pillow:masterfrom
tzjtan:master

Conversation

@tzjtan
Copy link

@tzjtan tzjtan commented Apr 2, 2019

Allow PDF exporting resolution to be specified as a tuple too, to get asymmetric DPIs sometimes used by sticker/inkjet printers, e.g 600x1200dpi.

tzjtan added 2 commits April 1, 2019 22:00
Allow PDF exporting resolution to be specified as a tuple too, to get asymmetric DPIs sometimes used by sticker/inkjet printers, e.g 600x1200dpi.
@tzjtan tzjtan closed this Apr 3, 2019
@radarhere
Copy link
Member

Hi. Out of curiosity, why did you close this?

@tzjtan
Copy link
Author

tzjtan commented Apr 3, 2019

Sorry, this is my first time sending a pull request and many things are confusing to me.
Let me learn a bit more git and retry this properly without the failed tests.

@radarhere
Copy link
Member

Okay, sure. Well, your tests were only failing on style points. So

if not isinstance(resolution,(list,tuple)):
        resolution2D = (resolution,resolution)

needed a space after the commas -

if not isinstance(resolution, (list, tuple)):
        resolution2D = (resolution, resolution)

I would have also suggested that you talk about width and height resolution, instead of 1D and 2D, unless that's an established convention. The only other things would have been providing a link in the discussion to some documentation showing the need for these different resolutions, and adding a test to https://github.com/python-pillow/Pillow/blob/master/Tests/test_file_pdf.py.

@tzjtan
Copy link
Author

tzjtan commented Apr 3, 2019

Ok thanks, I have spotted the styling errors and got tests to pass on Travis CI.
I have also added some tests to test_file_pdf.py as you suggested, and modified the documentation at docs/handbook/image-file-formats.rst.

Please let me know if some things should be done another way in my modified pull request.

@radarhere
Copy link
Member

This is being attempted again in PR #6961

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