Skip to content
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

feat: Add :delete_temporary_files flag #1068

Merged
merged 5 commits into from
Aug 23, 2023

Conversation

crespire
Copy link
Contributor

Adds an option to keep the temporary files generated by pdf_from_string method. It defaults to using the close! handler in the ensure block.

@crespire
Copy link
Contributor Author

For compatibility purposes, we might consider switching the option to delete_temp and adjusting the logic to match as that's the current default behaviour. Happy to do so, let me know!

@unixmonkey
Copy link
Collaborator

unixmonkey commented Aug 18, 2023

Yeah, I think I'd prefer the option to be delete_temporary_files: true, and check the value rather than key presence (someone might set it to something that evaluates to false), & use a conditional rather than send something more like this:

if options[:delete_temporary_files]
  file.close!
else
  file.close
end

What do you think?

@crespire
Copy link
Contributor Author

Yeah, I think I'd prefer the option to be delete_temporary_files: true, and check the value rather than key presence (someone might set it to something that evaluates to false), & use a conditional rather than send something more like this:

if options[:delete_temporary_files]
  file.close!
else
  file.close
end

What do you think?

I think that's very sensible! I'll make the changes.

@crespire crespire changed the title feat: Add keep_temp flag feat: Add :delete_temporary_files flag Aug 22, 2023
@unixmonkey
Copy link
Collaborator

Could you please add this new option to the README's list of "all options" please?

@crespire
Copy link
Contributor Author

crespire commented Aug 22, 2023

As an entry in this section? https://github.com/mileszs/wicked_pdf#super-advanced-usage

@unixmonkey
Copy link
Collaborator

I was thinking under this line here: https://github.com/mileszs/wicked_pdf/blob/master/README.md?plain=1#L262

@unixmonkey unixmonkey merged commit 6b3b90f into mileszs:master Aug 23, 2023
12 checks passed
@unixmonkey
Copy link
Collaborator

Great stuff! Thank you.

@crespire crespire deleted the feat/keep_temp_flag branch August 23, 2023 18:38
@unixmonkey
Copy link
Collaborator

This is now in version 2.7.0 of wicked_pdf. Thank you for your help!

@crespire
Copy link
Contributor Author

This is now in version 2.7.0 of wicked_pdf. Thank you for your help!

Thank you for your guidance! Happy to have contributed.

@jjb
Copy link

jjb commented Sep 15, 2023

Thanks for what seems to be a nice feature

maybe there could be more docs on when/why this is useful

without this, the temp files will be cleaned up during GC, right?

so it's useful maybe for someone creating pdfs at high volume, where the disk files might add up quickly?

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.

3 participants