search

walle / gimli

Utility for converting markup files to pdf files
https://github.com/walle/gimli
MIT License
538 stars 44 forks source link

Create cover html by Tempfile #66

Closed k0kubun closed 9 years ago

k0kubun commented 9 years ago

Fixes https://github.com/walle/gimli/issues/63 ref: https://github.com/walle/gimli/pull/48

I could reproduce https://github.com/walle/gimli/issues/63 by installing gimli to system ruby or changing config directory's owner to root. As I said here, there is no need to create temporary html file in gem's internal directory. It'll cause a permission denial if gimli is installed to system ruby.

I changed the implementation using Tempfile. Because wkhtmltopdf requires a cover file to have .html extension, I added .html extension to Tempfile.

walle commented 9 years ago

@k0kubun we have two pull requests that seem to fix the same issue, and I don't really know which one to use. The other solution is https://github.com/jchaves/gimli/commit/517ce74fd78b5f2e57768bbdb6d4cbe215685794

What are your comments? @k0kubun @jchaves

k0kubun commented 9 years ago

https://github.com/jchaves/gimli/commit/517ce74fd78b5f2e57768bbdb6d4cbe215685794 creates a temporary file to the current directory and does not remove it. My implementation creates one in a temporary directory, such as /var/folders/... in OSX. I think https://github.com/jchaves/gimli/commit/517ce74fd78b5f2e57768bbdb6d4cbe215685794 will work well but using Tempfile is better for this case.

jchaves commented 9 years ago

I strongly encourage you to use @k0kubun 's code. Mine was just a quick patch to get it to work, but it's ugly and dirty, and it's only a consecuence of my lack of ruby knowledge.

I'm reading ruby-doc on class Tempfile, and I think it's the logically right solution for this case. So go on, and please use @k0kubun solution.

walle commented 9 years ago

This is merged and released in 0.5.8. Thank you for your contributions!