Add support to merge files to one pdf #3

Closed
walle opened this Issue May 11, 2011 · 7 comments

2 participants

@walle
Owner

Add a -m (merge) flag to support merging all files to one.
This should perhaps allow us to specify a list of files to convert/merge instead of just one or all in the folder.

@nicknovitski

I made a topic branch that merges all matched files, but it's based on the Converter class being changed (converted, you could say) to taking an array of MarkupFiles instead of one each: njay@5522f2b

It's set up so the old tests still pass, but I was wondering what your thoughts would be on changing the API in that way. It seems like a better deal to me, especially with the merge functionality in mind: converting the files all at once seems better than tracking, merging and cleaning up the pdfs after-the-fact (even though having page-breaks between each source file would be pretty nice (oh boy, a new option to code)).

@walle
Owner

I think making the converter take an array of MarkupFiles is the way to go. It should have been designed that way from the beginning.

I think we should add another option to explicitly set the name of the output file.

The page break functionality does not seem to hard to code, I have only skimmed through the cast but here is a description on how to do it. http://asciicasts.com/episodes/220-pdfkit
It would be nice to add a class that does the page break so you can use it in your markup too.

I will take a closer look on your branch when I get home tonight.

Thanks for your work on the project!

@walle
Owner

I have made some commits in https://github.com/walle/gimli/tree/merge. The branch is based on yours.

@nicknovitski

Everything looks good, except in 6050e92, I'm pretty sure that the check to make sure files is an array is unnecessary.

@walle
Owner

Good point. I'll fix that now.

@walle
Owner

This is merged to development. Closing this issue and opening new ones for the output filename and the page break class.
Thanks for your contribution!

@walle walle closed this May 29, 2011
@walle
Owner

This is now included in 0.1.7. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment