Let's call it if filename

Why do we need to pass .absolute here?

I would prefer something like:

tmp_file_path = ...
if filename_type == "str":
    filename = str(tmp_file_path)
elif filename_type == "file_object":
     filename = open(tmp_file_path, "w", encoding="utf-8")
else:
     filename = tmp_file_path

report.write_html(filename)
assert tmp_file_path.exists()

This looks great! In the issue I forgot to mention a couple of things:

• Sometimes generating the report can fail (unfortunately!). We want to make sure we close the file (if we opened it) in this case; also we may want to avoid truncating or creating it before we know we have something to write.
• The report will always set the charset to utf-8 in the page's metadata. So if the user passes an open file we may optionally check the file's encoding if we want to be extra cautious. Also note unless the doctring we write forbids it the user could pass a binary stream so we may want to handle that case, too.
• I'm not sure we need to enforce the filename suffix. I could imagine someone wanting to save to "report.htm" or "report.html.tmp" or something so I'd rather let the user name the file however they want.
• If we allow both a path and a file object maybe the parameter could be just "file" instead of "filename"?

So taking those details into account I guess the function could be modified slightly to look similar to this:

    def write_html(self, file):
        html = self.html()
        if isinstance(file, (str, Path)):
            with open(file, "w", encoding="utf8") as stream:
                stream.write(html)
            return
        try:
            file.write(html.encode("utf-8"))
            return
        except TypeError:
            pass
        if (encoding := getattr(file, "encoding", None)) is not None:
            try:
                assert codecs.lookup(encoding).name == "utf-8"
            except (AssertionError, LookupError):
                raise ValueError(
                    f"If `file` is a text file it should use utf-8 encoding; got: {encoding!r}"
                )
        file.write(html)

thank you comments @jeromedockes , I have couple of questions.

1. What are the reasons behind TypeError
2. if the encoding is None it seems the locale.encoding will be used, so we want to check for that too and raise an error if it is not utf-8 ?

Hi @mrastgoo ,

1. What are the reasons behind TypeError

Trying to write bytes and catching the TypeError is an easy way to detect if we are dealing with a text stream or a binary stream.

>>> 'é'
'é'
>>> 'é'.encode('utf-8')
b'\xc3\xa9'

>>> binary_stream = open('thefile', 'wb') # 'wb' = 'write + binary'
>>> binary_stream.write('é'.encode('utf-8')) # we can write bytes
2
>>> binary_stream.write(b'\xc3\xa9')
2
>>> binary_stream.write('é') # trying to write a string directly is a TypeError
Traceback (most recent call last):
    ...
TypeError: a bytes-like object is required, not 'str'
>>> binary_stream.close()

>>> text_stream = open('thefile', 'wt', encoding='utf-8') # 'write + text', or 'w' as text is the default
>>> text_stream.write('é') # we can write strings
1
>>> text_stream.write(b'\xc3\xa9'.decode('utf-8'))
1

>>> text_stream.write(b'\xc3\xa9') # trying to write bytes to a text stream is a TypeError
Traceback (most recent call last):
    ...
TypeError: write() argument must be str, not bytes
>>> text_stream.close()

>>> with open('thefile', 'w') as stream:
...    print(stream.mode)
w

>>> with open('thefile', 'wb') as stream:
...    print(stream.mode)
wb

>>> import io
>>> stream = io.BytesIO()
>>> stream.write(b'\xc3\xa9')
2
>>> stream.mode
Traceback (most recent call last):
    ...
AttributeError: '_io.BytesIO' object has no attribute 'mode'

2. if the encoding is None it seems the locale.encoding will be used, so we want to check for that too and raise an error if it is not utf-8 ?

If we pass encoding=None to open it will default to using locale.encoding, but AFAIK the resulting file object will always have an explicit encoding (not None).

>>> with open('thefile', 'w', encoding=None) as f:
...     print(f.encoding)
UTF-8

So I think it should be ok to inspect the encoding of the file object in this case.

>>> stream = io.StringIO()
>>> stream.encoding is None
True
>>> stream.write('é') # ok
1

>>> with open('thefile', 'w') as f:
...     report.write_html(f)

sorry for the long answer, hope this makes sense!

I fail to see the use case for allowing the user to pass a file instead of a filename. According to some path, this feature should address the simple use case of writing an HTML file to disk. Can you motivate this a little bit? It looks like plain over-engineering to me TBH.

I believe it was a suggestion from @glemaitre .

but if you look at functions offering to save some object as text such as matplotlib.savefig, pandas or polars to_csv etc, xml.etree.write, plotly write_html and others it's hard to find an example that does not allow passing a file object.

one example use case would be using a temporary file, testing, situations in which you may need to write either to an actual file or an in-memory buffer, ...

from personal experience a while ago in a completely unrelated context NiftiImages from the nibabel library could only be saved to a filesystem path and I can't remember what were the exact situation where it was a problem for me but I remember it was annoying

Thank you @jeromedockes for the explanation. it helped a lot.

if we want to be super exhaustive maybe we could test files both in text and binary modes?

maybe we could use the pytest tmp_path fixture?

we don't need to check the full content but maybe we could see that the file contains "</html>" to see that the full report has been written

this test is looking great! the last thing we need to take care of is to make sure we close the file if we opened it (otherwise we "leak" a resource: the opened file handle. that could be a problem for example to clean up the temp directory on windows as it refuses to remove files that have an open file handle). Usually we ensure that with a simple context manager like:

with open(tmp_file_path, 'w', encoding='utf-8') as file:
    file.write('hello')

but here we have a tricky situation because in some cases we have a string or path (which require no closing) and sometimes we have a file object which does require closing.

The standard library module contextlib provides 2 ways to deal with that situation easily. The first is ExitStack: it creates a context and we can push as many context managers as we want to its stack; when it exits it unwinds the stack, calling each manager's __exit__ when it is popped. So we could use it like:

with contextlib.ExitStack() as stack:
    if file_type == 'str':
        file = str(tmp_file_path)
    elif file_type == 'text_file_object':
        file = stack.enter_context(open(tmp_file_path, 'w', encoding='utf-8'))
    # ...

    report.write_html(file)

# if we opened it the file is closed here when we exit the `with` block

This option using ExitStack is my favorite because the file is being managed by a context manager as soon as it is opened.

Another way is to use nullcontext in the cases where we do not open the file, so that later we can treat all options as if they were open files that implement the context manager protocol. nullcontext returns an object that implements the context manager protocol but whose __enter__ just returns the object we gave it and __exit__ does nothing:

if file_type == 'str':
    file = contextlib.nullcontext(str(tmp_file_path))
elif file_type == 'text_file_object':
    file = open(tmp_file_path, 'w', encoding='utf-8')

with file:
    report.write_html(file)

wow contextlib.ExitStack() is very nice

Maybe you could add a comment on L139 to explain why it's a good idea to use contextlib here?

here also we want to use a context manager to close the file

otherwise the assert is never executed because of the exception raised on the line above

I actually change this to check if the file doesn't contain html to make sure we don't modify it.

Could you add a comment explaining what html is expected (or not expected) to be at line 230?
Additionally, light inline documentation/comments on the steps above would help readability :)

Maybe you could add a comment on L139 to explain why it's a good idea to use contextlib here?