Fix Issue 17844 - std.process.execute should allow not capturing stderr

[CyberShadow]

No description provided.

[dlang-bot]

Thanks for your pull request, @CyberShadow!

Bugzilla references

Auto-close	Bugzilla	Description
✓	17844	std.process.execute should allow not capturing stderr

[CyberShadow]

Picking a good name for this flag is a bit tricky, because there is a retainStderr flag with a different meaning. Not too happy with passStderr because of the consecutive s letters.

[schveiguy]

Ugh, I hate the way this is specified, regardless of the name. In hindsight, stderr should not have been automatically redirected without being told to do so.

When I first read passStderr without ingesting the whole change and bug report, I thought it was a way to pass a specialized stderr to the child. Really what it should be is passThroughStderr, or maybe ignoreStderr. What you are doing is leaving the stderr setup alone.

Anyway, that's my 2 cents. IMO, the default should be to ignore stderr in the output, as the bug report says, but of course, that may break people's expectations and code.

Another option is to reverse the boolean meaning and make it a positive. i.e. includeStderr. Then you make the config default value Config.includeStderr. This will only break if someone both needed to specify parameters after the config parameter (and likely passed Config.none), or they needed to specify config values to the pipe function, AND they expected stderr output to come back in the return value. Likely breaking any code is not worth considering.

Another more drastic option is to rewrite the API and change the function names, going through a deprecation cycle to get people onto the correct API.

In any case, I can live with the name as you have it, it's terrible, but not many good options.

[CyberShadow]

In hindsight, stderr should not have been automatically redirected without being told to do so.

Agreed.

When I first read passStderr without ingesting the whole change and bug report, I thought it was a way to pass a specialized stderr to the child. Really what it should be is passThroughStderr, or maybe ignoreStderr. What you are doing is leaving the stderr setup alone.

Interesting you should mention those name suggestions. My initial idea was ignoreStderr, however I thought that one obvious interpretation of that name implies that anything written to stderr would be completely discarded. My second idea was passThruStderr, similar to the PHP passthru function, but I thought it was too long, and the function's effect in PHP when processing a HTTP request (PHP's most common execution context) is rather different from how stderr is typically encountered by D programs.

We can go with passThroughStderr I guess, unless there are better ideas?

[schveiguy]

passThruStderr is shorter, go with that.

[dnadlinger]

@schveiguy: thru is a "chiefly North American informal spelling" and looks just wrong to my eyes. I'd argue that three extra characters in an infrequently used option is well worth it for not violating the principle of least surprise. (Other parts of Phobos use passThrough etc., but thru isn't used in user-facing code.)

[CyberShadow]

thru is a "chiefly North American informal spelling"

Well, there's industry precedent with passthru.

in an infrequently used option

Did you read the above discussion and bug report? If we could make it the default, we would.

I'm not going to change the name again without a substantial argument.

[schveiguy]

It's OK to use passThroughStderr instead, but I disagree with the objections. The question to answer is: can passThru mean something other than "pass through"? If not, it's not surprising or confusing. It's just a standard quirky abbreviation that programming libraries and languages are full of.

[dnadlinger]

in an infrequently used option

Did you read the above discussion and bug report? If we could make it the default, we would.

Of course I did. On average, the name will maybe be used in, what, one in every 10 000 lines of D code, at most? That's not frequent enough to try and be cute just to save those three characters.

I'm not going to change the name again without a substantial argument.

Your initial proposal from a few comments up was passThroughStderr as well. Just because you got talked into changing it too readily doesn't somehow make it the right thing to do.

---

The question to answer is: can passThru mean something other than "pass through"? If not, it's not surprising or confusing.

That implication is fallacious. The situation where it is confusing is not when reading code, but when writing it. You are looking for a parameter you probably remembered as "pass through stderr". Since we've by and large done quite a good job with keeping the names consistent in Phobos, you can be reasonably sure that it is going to be named passThroughStderr.

The spelling "pass-thru" isn't in Merriam-Webster, while "pass-through" is. I'm not sure how there is even a discussion about this. At best, the two alternatives are equivalent (three characters saved out of Config.passThrough is hardly an argument either way), but one is informal and potentially confusing, the other is not.

If you are regularly working on code that handles subprocesses in D, you would of course remember the spelling soon enough. But when switching often between codebases and languages, it's exactly this sort of inconsistency that causes unnecessary friction.

[schveiguy]

The situation where it is confusing is not when reading code, but when writing it. You are looking for a parameter you probably remembered as "pass through stderr".

I don't think I would remember either name, even if I'm writing it often.

Either way, it's a poor way to name it. It should really be the opposite (opt-in to receiving stderr output, not opt-out).

I think we've spent too much time already debating it, just pick one and merge.

[CyberShadow]

That's not frequent enough to try and be cute

Nobody is trying to be cute.

Just because you got talked into changing it too readily doesn't somehow make it the right thing to do.

Nice attack on my character drawn out of an assumption.

I have limited time and patience to go back-and-forth on the same nitpicking matters, as should everyone.

The spelling "pass-thru" isn't in Merriam-Webster, while "pass-through" is.

Neither is writeln or many other names in Phobos. We use them because they are common in programming.

Meanwhile, passthrough is not a function in PHP, while passthru is.

That implication is fallacious.

So was much of your post. It was very frustrating to read.

Since you feel so strongly about the current name, I am going to change the name to stderrPassThrough (to also disambiguate from passing something through stderr). I suggest that you use your time to contribute more constructively in the future, though.

[schveiguy]

I'm assuming the name is good now.

[kyllingstad]

I realise that I am somewhat late in reviewing this now (sorry!), but I would like to add my comments anyway. And the first thing that comes to mind is that this actually changes something fundamental about Config: It is no longer just a set of flags that control the behaviour of spawnProcess() and spawnShell(); now we have a flag which uniquely affects execute() and executeShell(). Is this OK? (I'm not saying it isn't, just that it is worth some thought.)

Assuming it is, my second comment is that this change should be reflected in the main Config documentation, which currently specifies that the flags apply to spawnProcess() and spawnShell().

Thirdly, the documentation for the new flag should then specify that the flag actually has no effect on spawnProcess() and spawnShell().

Again, sorry to be Johnny-come-lately here.

[schveiguy]

All excellent points. I think the reason for using Config is because it's already being passed to execute and executeShell. Adding a new set of flags just for those functions would be detrimental and confusing. Adding a simple bool flag at the end (or Flag) would be ugly too.

In reality, the best way to have fixed this (in hindsight) is to not have the automatic redirection of stderr to stdout, and let the user specify it in the config in the first place. But it's hard to undo that at this point.

We should definitely update the docs to reflect the special nature of the new flag.

[CyberShadow]

Thanks Lars, good suggestions. I'll send a follow-up PR.

[CyberShadow]

-> #5744