postgre: add more error information to an error message (position, line number, partial SQL) #204
Conversation
…tes so repo is always checked out as unix line endings for files (even on Windows)
|
@amacneil Is this okay to merge in? If not, I don't have a personal stake in this being merged in so I'm fine with this being closed and unmerged. (I feel like the code solution could be less complicated / better but I have no idea how nor do I have the time to explore it) |
amacneil
force-pushed
the
feature/add-detailed-error-message
branch
2 times, most recently
from
d3a0e3a
to
ed0cef1
Compare
| Position int | ||
| } | ||
|
|
||
| var _ error = new(DetailedSQLError) |
There was a problem hiding this comment.
explicitly asserts at compile-time that DetailedSQLError satisfies the error interface.
https://stackoverflow.com/a/34619122/5013410
|
Just wondering, was there a reason this didn't get merged? |
|
I wanted to make some changes to how it was implemented, which I started doing locally, but haven't had much time for dbmate development lately. It's not out of date or anything and I still think would be worth trying to land at some point. |
|
This looks good to me. If you want to tweak things, @amacneil, it could be done as a follow-up. It doesn't have to perfect. |
| # Use Unix line endings in all text files. | ||
| * text=auto eol=lf |
There was a problem hiding this comment.
Not a big fan of requiring this for all files in the repo.
| @@ -4,5 +4,5 @@ | |||
| /db | |||
| /dbmate | |||
| /dist | |||
| /testdata/db/schema.sql | |||
| /testdata/*/db/schema.sql | |||
There was a problem hiding this comment.
I'd prefer we leave the legacy test fixture files as-is, and instead use the testing/fstest virtual FS for future test cases that require files in their test fixture. Doing so would reduce the amount of change required to implement this PR.
| // NewDetailedSQLError creates a structure that computes the line and column of an SQL error based solely on position | ||
| func NewDetailedSQLError(err error, query string, position int) *DetailedSQLError { | ||
| column := 0 | ||
| line := 0 | ||
| itColumn := 0 | ||
| for _, c := range query[:position] { | ||
| if c == '\r' { | ||
| // ignore carriage return (Windows CRLF formatted files) | ||
| // when counting column | ||
| continue | ||
| } | ||
| if c == '\n' { | ||
| // add 1 as column count starts at 1 (not 0) in most text editors | ||
| // and other tools | ||
| column = itColumn + 1 | ||
| itColumn = 0 | ||
| line++ | ||
| continue | ||
| } | ||
| itColumn++ | ||
| } | ||
| return &DetailedSQLError{ | ||
| SQLError: err, | ||
| Line: line, | ||
| Column: column, | ||
| Position: position, | ||
| } | ||
| } |
There was a problem hiding this comment.
There should probably be a few tests added to exercise the various scenarios (line endings, UTF-8 vs. plain ASCII SQL).
|
Given that this issue (originally #102) has recently celebrated its fourth birthday 🎂, I took a stab at reviewing this PR, and left review comments above. Rather than burden the OP with bringing their branch up to date with all the changes since the PR was created, as well as implementing any requested changes from my review, I did the work and submitted PR #495. I'd like to propose closing PR #204 in favor of merging PR #495, and thank @silbinarywolf for their initial effort which I borrowed from conceptually. How do people feel about my proposal? |
Historical context:
Note from exploring this change:
Fixes #102