html2image icon indicating copy to clipboard operation
html2image copied to clipboard

Added method to remove temporary files created

Open TytoCapensis opened this issue 2 years ago • 2 comments

Hello,

I added the method _remove_tmp_file which is supposed to remove the temporary files created when we supply a HTML string to screenshot() instead of providing a file. This prevents too much files from accumulating in the temporary directory.

Feel free to change the name of the method or to tell your opinions about this PR :)

TytoCapensis avatar Feb 18 '22 15:02 TytoCapensis

Hello, thanks for opening a PR, something like this is indeed missing from the package.

It could almost be merged as-is, but I think one should be able to choose whether or not they want the files to be automatically deleted. I can think of two reasons:

  • If something went wrong with the screenshot process, taking a look at the HTML files in the temporary directory by opening them in a browser can be a way to see what the pages should look like.
  • There could be some use-cases where one have to take a lot of screenshots and would like the process to finish as soon as possible. In this case, the deletion time (even if insignificant compared to the time it takes to take a screenshot) could adds up while taking screenshots, whereas the files could be deleted later without adding time to the screenshot-taking process.

Here is how I would add the possibility to toggle the automatic deletion:

  • add an auto_remove_tmp_files (or a similar name) parameter to the Html2Image class' __init__ with a default value of True
  • add an attribute with the same name to the Html2Image class
  • add a simple if statement surrounding the call of the _remove_tmp_file method
  • and finally add docstring and mention the attribute/parameter in the readme

On a side note : as-is, this method only deletes HTML files and leaves CSS files untouched. It is not necessarily bad however : a CSS file can be loaded one time and be used by an infinite number of HTML files, so deleting them may sometimes be counter-productive.

Still, being able to delete everything in the tmp folder using one command would be nice, it could be a function that would have to be called manually.


Anyway, thanks again for your PR. I wrote everything that came to my mind to make sure that I won't forget about it.
If you wish to add anything that I mentioned in this comment, feel free to do so. If not, I'll add it later myself to this PR.

I'm currently focusing on making this package compatible with Chrome's and Firefox's CDP interface (which is a way to control a web browser via websockets), so any unrelated changes will come after that, sooner or later.

vgalin avatar Feb 18 '22 21:02 vgalin

I added an improvement commit:

  • method is now called _remove_temp_file (for better consistency with temp_path)
  • an argument keep_temp_file is now present in the class constructor and initialized to False
  • added documentation in docstring and README

However, I am unsure to understand what you mean with CSS files: from my perspective, it looks like that only HTML files are created, since the CSS string we pass to the screenshot function is embedded into the HTML file. Are there particular circumstances in which temporary CSS files (and anything else than HTML) are supposed to be created?

TytoCapensis avatar Mar 17 '22 13:03 TytoCapensis