Added the ability to overlay one PDF over another #63
Conversation
|
Thanks for doing this, I'll have a look over the weekend! |
| (.setEvenPageOverlayFile overlay even-page-overlay-file-path) | ||
| (.setEvenPageOverlayPDF overlay even-page-overlay-pdf) | ||
|
|
||
| (-> overlay (.overlay {}) (.save output-file-path)) |
There was a problem hiding this comment.
Apologies for hijacking the thread, I thought I'd offer some tips... Hope it's ok 🙂
So yeah, this is dotemacs' call but you might want to avoid saving to disk since that's a side-effect (and side-effects make writing proper tests harder, among other things)
I'd also argue that returning a byte array would be more useful to the end-user. Every user of your function is interested in the output, one way or the other, but how many will be interested in storing it locally right away?
There was a problem hiding this comment.
No problem. Thank you for the feedback. I agree a byte array would be more useful in general but would mean its different from the other functions for example draw, form and merge all save to disk.
I'm happy to have it either way. @dotemacs is there reason to save to disk here or can I change this to return a byte array?
| (deftest pdf-file-merge | ||
| (let [_ | ||
| (overlay-pdf | ||
| {:input-file-path "test/pdfs/clojure-1.pdf" | ||
| :overlay-file-path "test/pdfs/clojure-2.pdf" | ||
| :output-file-path output-file-path}) | ||
|
|
||
| overlayed-pdf-file (.exists (io/as-file output-file-path))] | ||
| (is (true? overlayed-pdf-file)) | ||
| (is (true? (common/is-pdf? output-file-path))))) | ||
|
|
||
| ;; clean up | ||
| (defn clean-up [file] | ||
| (if (.exists (io/as-file file)) | ||
| (io/delete-file file))) | ||
|
|
||
| (deftest cleaner | ||
| (clean-up "test/pdfs/foo.pdf")) |
There was a problem hiding this comment.
Some more feedback here as well!
- Keep in mind that not all test runners are deterministic! I don't know about clj test-runner but others like kaocha allow you to randomize the test run sequence to catch side-effects. Therefore you should not make assumptions about the order tests run. Consider cleaning up the temporary file in the deftest it was created ;).
- What if the output file already exists for some reason unrelated to these tests? Your tests will overwrite that and then delete it. That shouldn't happen. As an alternative, take a look at this: https://stackoverflow.com/a/57845906
You could also consider the following function:
(defn with-temp-pdf [prefix fn]
(let [file (java.io.File/createTempFile prefix ".pdf")]
(fn (str file))
(if (.exists file)
(io/delete-file file))There was a problem hiding this comment.
Very good points! I followed this pattern from the tests in merge.
I'll update once the byte array vs output file is finalised.
Description of your pull request
Added the ability to overlay one PDF over another
Pull request checklist
Before submitting the PR make sure the following things have been done
(and denote this by checking the relevant checkboxes):
clj-kondo --lint src).Thanks!