con / duct

A helper to run a command, capture stdout/stderr and details about running
MIT License
1 stars 1 forks source link

What is the point of `temp_output_dir`? #79

Closed jwodder closed 2 weeks ago

jwodder commented 2 weeks ago

The temp_output_dir fixture in the tests is just the same as tmp_path, except:

  1. It converts the path to a str and appends an os.sep. If the goal of is to allow you to join paths by just doing temp_output_dir + filename, I contend that this is bad because you should instead be using pathlib.Path objects joined with the / operator (or at the very least os.path.join; use the tools Python gives you!). Moreover, if you're worried about mixing Path with str, that's what type-checking is for.

  2. It deletes the test dir at the end of the test. Unless the contents of these directories are so huge that you can't have more than one on your hard drive at once, this is unnecessary, as pytest already manages cleanup of test directories.

I therefore propose eliminating temp_output_dir and just using tmp_path instead.

CC @asmacdo

asmacdo commented 2 weeks ago

The why:

--output-prefix is considered a prefix to the filename unless it explicitly ends in /. I did this so we don't have to add the separator in each test.

Removing the temp dirs at the end of the test was actually useful because it failed if I had any dangling threads that would attempt to write after the command executed.

But I definitely don't mind if you have a more idiomatic way. (The code is really inconsistent with how I handle paths, Ive used os.path, pathlib, and string ;)