Lazy open output files, and always close them

[kurtmckee]

This PR modifies the test suite to show all warnings, including ResourceWarning. By doing so, this revealed that the CLI code never closes output files opened by argparse during argument parsing.

Therefore, this PR additionally modifies the CLI code to open output files only when they will be written to, and ensures that they are always closed after writing.

[kurtmckee]

Rebased on main

[miketheman]

One question inline, but otherwise looks fine.

I'm curious about the impact of leaving open files in a CLI tool - don't those descriptors disappear once the runtime exits?

Considering the primary use cases for this project is a library, how important is this change?

[kurtmckee]

Great questions.

• I'm curious about the impact of leaving open files in a CLI tool - don't those descriptors disappear once the runtime exits?

  Yes, they will. However, I had two considerations when I observed this.

  The first is a recent experience at work with a tool that expected to be run as a CLI but that we needed to import, customize, and call the cli.main() function ourselves. That recent experience made me want to address this as a hygiene/best practice consideration.

  The second is a functional consideration: the output file is created even if there's an error.

  For example, if nh3 raises ValueError, that is caught, None is returned, and sys.exit(1) is subsequently called. An output file is created even though rendering failed.

  This PR changes that behavior so an output file is only created when rendered output is ready to be written, and ensures that resources are cleaned up afterward.

• how important is this change?

  Not important. I think it's a low value -- but correct -- change.

• Curious why you went with the outer call instead of using pytest -Wall ...?

  I didn't know that -Wall existed! 😀

  I have had past experience that configuring pytest's filterwarnings to raise errors on warnings wouldn't catch errors in very specific ways, like a DeprecationWarning that was triggered by Python 3.12 when importing an old version of dateutil; the solution I found at that time was to switch from pytest's filterwarnings to invoking Python directly, and that's the solution I reached for here.

  Want me to switch to pytest -Wall instead of python -Wall -m pytest?

[miketheman]

... we needed to import, customize, and call the cli.main() function ourselves ...

Thanks for the context, and that makes sense.

The library was never fully intended as a CLI, so please be mindful of the cli namespace, as it is entire possible to change (as we ware changing it here). I'd be open to understanding the utility performed in the CLI that should be part of the general API, and thus decrease usage on cli.*

This PR changes that behavior so an output file is only created when rendered output is ready to be written, and ensures that resources are cleaned up afterward.

This makes most sense here.

Want me to switch to pytest -Wall instead of python -Wall -m pytest?

Yes please! If we have a specific reason to move it to the "outer" behavior you've described, then we can do that at a later time, but we should use the built-in if possible.

[kurtmckee]

Oh no, my commit signatures appear to have been broken by using GitHub's web UI to rebase on main. Let me circle back to this when I'm back at my PC tomorrow.

Regarding the cli.main() discussion -- I wanted to be clear that this was not something involving this project, it was just an example from my recent experience that "people use stuff in unexpected ways". 😀

[miketheman]

Thanks! Merging now.

If you'd like to craft a CHANGES.rst entry for 44.0, we can get these released. Here's an example of a previous PR for releases: #303