[fact] Remove wrapper for capturing output of subprocess

[algomaster99]

I feel the wrapper captured_run is not required because the output was actually captured in just one method. Moreover, the parameters of subprocess.run are not a lot and quite explanatory of what we are trying to achieve.

[slarse]

Hi @algomaster99 ,

Your idea is not bad, but you've missed a side effect of capturing the output, see comment.

[slarse]

I think you're missing something here. If the output is not captured, then it will go to stdout/stderr. That means that this function now causes the output of git init to be displayed to the user, which is not desirable.

[algomaster99]

Oh, I thought the default behaviour would be to discard the output. In more technical terms, it would redirect the output to /dev/null. Since we do not care about the output here, should we do redirect the stdout and stderr to /dev/null?

[slarse]

Oh, I thought the default behaviour would be to discard the output.

A child process inherits the file descriptor table of its parent process (this is consistent across all operating systems and programming languages I know of), so if the parent process outputs to stdout and stderr, so will the child.

Since we do not care about the output here, should we do redirect the stdout and stderr to /dev/null?

We could do that, but the benefit over just using captured_run isn't all too clear to me. Perhaps there's something different we can do here? Rename the function perhaps?

[algomaster99]

I feel that using subprocess.run is very clear by itself given it is a common python function and we have passed very few arguments to it. If it needed a lot of extra arguments, a wrapper would be desirable in that case.

When I saw this function, my first thought was to reuse this everywhere where subprocess.run was used. However, it was definitely not needed because of the same arguments above. Moreover, for this specific case, we don't even want to capture output so a function name captured_run shouldn't be invoked in my opinion.

[slarse]

I feel that using subprocess.run is very clear

It's not that it isn't clear, it's just an incredibly noisy function that takes way too many arguments.

Moreover, for this specific case, we don't even want to capture output so a function name captured_run shouldn't be invoked in my opinion.

So, originally there were two functions: captured_run and quiet_run, I removed quiet_run because it was semantically identical to invoking captured_run without using the captured output. This is why I suggested a rename of the function such that it's less confusing, because semantically this is what we want. Whether or not the output goes to devnull or to a temporary buffer that's then flushed doesn't matter.

[algomaster99]

it's just an incredibly noisy function that takes way too many arguments.

In our case, we just pass four arguments and two of them are common - stdout and stderr. In my opinion, it was not enough for me to create a wrapper around it.

This is why I suggested a rename of the function such that it's less confusing

It is not confusing at all. I wanted to use captured_run wherever we invoke subprocess.run for the sake of uniformity. In most of the places where the latter is invoked, the same set of arguments is provided. However, there were a lot of occurrences of subprocess.run and they were readable as well. Thus, I thought to remove captured_run instead to achieve the goal I thought of.

[slarse]

It is not confusing at all.

To some extent it has to be confusing, as in refactoring, you inadvertently changed the semantics of the program by letting the output go to stdout/stderr.

Anyway, there's a neater option available as of #986, because in Python 3.7+ there's a capture_output argument to subprocess.run. So we can just set capture_output=True, which would more conclusively obviate the need for a wrapper function.

[slarse]

@algomaster99 Now we can actually make this update more objectively worthwhile. As we've now dropped support for Python 3.6, we can use the capture_output parameter to supbrocess.run to capture output :)

[algomaster99]

@slarse

make this update more objectively worthwhile

I agree. The wrapper is no longer needed now :)

[algomaster99]

@slarse The failing test passes on my local so it is making it harder to debug. Do you have an idea what could be the reason?

[slarse]

It failed for me with Python 3.10.1, but it seems unrelated to your changes. Something probably changed with asyncio going from 3.10.0 to 3.10.1.

I've pushed a potential fix, let's see if it works :)

[slarse]

Indeed, get_event_loop was deprecated in 3.10: https://docs.python.org/3/library/asyncio-eventloop.html#asyncio.get_event_loop

But tests are still failing. I'm relatively certain it's not due to your changes. My lunch hour is up soon so I'll have to return to this tonight :)

[slarse]

Hmm, so this is now passing for me locally. Not quite sure what the problem is.

[slarse]

Pretty sure this has something to do with the version of Git and how the repos are initialized. This error message is prevalent:

fatal: couldn't find remote ref HEAD

But the expected error message is with an uppercase c:

fatal: Couldn't find remote ref HEAD

So they probably made it lowercase at some point. I just need to casefold here and it should be fixed.

[algomaster99]

Thanks for debugging it :)
I think this is ready for merge? We can ignore the complaint by codecov.

[slarse]

Agreed, thanks @algomaster99 :)