-
Notifications
You must be signed in to change notification settings - Fork 38
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 support for exporting pdf to image #56
base: master
Are you sure you want to change the base?
Conversation
Hi @werenall thank you for the PR. I'd like to think about this a little bit. But from a quick look: you seem to have extended the functionality of an existing function and added a new feature in the same commit message. I think that this should be split in separate commits. This is just from a quick glance. |
Yeah, I agree. Take all the time you need and just let me know in a bulk what I can improve. |
To summarise: The changes in The same with I'd say that only the addition of Since you mentioned in #55 why you've created it, I have some context, but maybe you can add an example on how it can be used in the README? For me to add this PR, it would be nice to see the following:
Thank you for your work |
c1d2a7c
to
d5d2713
Compare
[Re dotemacs#55] This commit also changes slightly the prerequisities for split function Previously it only allowed strings as inputs. IMHO it should also accept files.
d5d2713
to
25eff3f
Compare
Ok @dotemacs, I think I resolved all your comments. Let me know if you still would like some fine-tuning. |
how is this PR going? i'm looking into this feature too |
HI @dotemacs, I wonder if you plan to merge this PR? |
Gentle ping |
Implement a namespace for exporting a PDF or PDDocument to a BufferedImage
[Re #55]
This PR also slightly changes the prerequisites for split function
Previously it only allowed strings as inputs. IMHO it should also
accept files.