Skip to content

Collect all error messages for Error.Error() #250

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Collect all error messages for Error.Error() #250

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion error.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package mssql
import (
"database/sql/driver"
"fmt"
"strings"
)

// Error represents an SQL Server error. This
Expand All @@ -24,7 +25,14 @@ type Error struct {
}

func (e Error) Error() string {
return "mssql: " + e.Message
var msg strings.Builder
for i, err := range e.All {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ge e.All

might last-to-first ordering be preferable? Similar to how exceptions get printed outer-to-inner

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that the natural order is the correct one, eg:

mssql: .... actual reason ...
mssql: Failed to drop constraint. See previous error.

So, it is like error log printed when you run an sql query in your terminal.

But I am not sure and open for any suggestions. The main problem I am trying to solve as stated in the PR description is to make Error() return enough details to take action.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your example implies there are Error objects without a Number. Where will See previous error show up?

if i > 0 {
msg.WriteByte('\n')
}
msg.WriteString(fmt.Sprintf("mssql: %s (%d)", err.Message, err.Number))
}
return msg.String()
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how is this change not affecting any tests? Is there really no coverage of this call?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. There is no tests which would check how the string returned by Error() string is formed. Do you think we need them?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

without a test, any behavior is considered correct. I think having a test now is worthwhile because it documents the contract that callers can expect.


func (e Error) String() string {
Expand Down