Distinguish non-error output from error output #122
Conversation
In certain cases such as non-interactive mode it was observed that error messages meant for error stream were captured under output stream. This is not desirable if the user only wants to retrieve the error messages. This commit aligns the go-sqlcmd behaviour with ODBC sqlcmd where error messages are captured in the appropriate error stream.
| if iactive { | ||
| _, _ = s.GetOutput().Write([]byte(err.Error() + SqlcmdEol)) | ||
| } else { | ||
| _, _ = s.GetError().Write([]byte(err.Error() + SqlcmdEol)) |
There was a problem hiding this comment.
Don't you need to distinguish the case of iactive being true because of the user name parameter being passed in, which is part of your other PR?
There was a problem hiding this comment.
maybe Sqlcmd should have a property like "WriteErrorsToErrorStream bool" that would be set by main before it calls Run.
There are some other settings that control command output destination that this could be like.
There was a problem hiding this comment.
Added flag in sqlcmd struct to indicate an interactive session.
The interactive session will be determined when the first input is scanned.
There was a problem hiding this comment.
would it make sense to fix s.GetError to do the right thing based on context and just change this code to use GetError() only?
I'm not 100% certain that every kind of error in sqlcmd goes to the same stream, though.
| @@ -109,6 +109,28 @@ func TestSqlCmdQueryAndExit(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestSqlCmdOutputAndError(t *testing.T) { | |||
| s, outfile, errfile := setupSqlcmdWithFileErrorOutput(t) | |||
There was a problem hiding this comment.
don't you need to test the s.IncludeFile path too?
There was a problem hiding this comment.
Added test for s.IncludeFile
| return s | ||
| } | ||
|
|
||
| func (s *Sqlcmd) scanNext() (string, error) { | ||
| s.IsInteractiveSession = true |
There was a problem hiding this comment.
add a comment on why this is being set
| @@ -86,10 +87,13 @@ func New(l Console, workingDirectory string, vars *Variables) *Sqlcmd { | |||
| s.PrintError = func(msg string, severity uint8) bool { | |||
| return false | |||
| } | |||
| s.SetOutput(os.Stdout) | |||
| s.SetError(os.Stderr) | |||
There was a problem hiding this comment.
This functionality change is leading to undesired side effects such as redirecting server errors to stderr as well. (I will close this PR and create new one)
I have reworked the changes to keep existing functionality as is in PR #143 and redirecting only the sqlcmd errors which is what the ODBC is also doing.
In certain cases such as non-interactive mode it was observed
that error messages meant for error stream were captured under
output stream. This is not desirable if the user only wants to
retrieve the error messages. This commit aligns the go-sqlcmd
behaviour with ODBC sqlcmd where error messages are captured
in the appropriate error stream.
Resolves #105
Validation: