Add check for difference between stdout/stderr streams #22
Conversation
There was a problem hiding this comment.
It appears that the current code may not be accurate, but it is a cost-effective solution. Performing all of the suggested actions on every logf call may be excessive. Could we consider moving the detection to the constructor and setting a field instead? This would be more efficient and still achieve the desired result.
|
Done. Just didn't want to introduce new state. |
logger.go
Outdated
| if outOk && errOk { | ||
| outStat, _ := outFile.Stat() | ||
| errStat, _ := errFile.Stat() | ||
| res.sameStream = os.SameFile(outStat, errStat) |
There was a problem hiding this comment.
omitting an error check bothers me here. I'm not sure in what situation Stat would fail (maybe a permission error or smth like this), but Stat returns an error, and I think it is better to check
Maybe it is also worth extracting the code to some isStreamsSame function, as it's getting too big
There was a problem hiding this comment.
What we gonna do if Stat fail? I think to throw panic, but not sure.
There was a problem hiding this comment.
panics from the library is not a good idea. just returns safe default, i.e. false
There was a problem hiding this comment.
fixed. Added error handling, extract to function and make some tests. Because I added new import, number of lines changed and some tests failed, so I had to change them
add error handling of os.Stat() with return safe default(false) add tests for isStreamSame
|
It failed tests as action has an old version of go. I'll fix it |
In current behaviour we have this check
l.stderr != l.stdout. By default, stderr and stdout issyscall.Stderrandsyscall.Stdoutwhich refer to differentfdbut actually refer to the same file(By default, they're probably connected to your terminal device), so this check have no sense in this context and by this reason we have two same log(attached screenshot) what wasn't intentable I think.I added check for difference between
stdoutandstderrto fix this behaviour.