Improve error handling in SQL parser #505
Conversation
| @@ -25,6 +24,37 @@ func FromBool(b bool) Direction { | |||
| return DirectionDown | |||
| } | |||
|
|
|||
| type GooseError string | |||
There was a problem hiding this comment.
I'm not convinced this makes things more readable.
The sql parser is an internal package, which means this error type isn't user-facing. Furthermore, we're not surfacing any specific information, i.e., it's still a string type which means asserting for this error would return the same error message as .Error. I think this adds more indirection.
|
I'm curious, was there a specific error you were running into with the sqlparser, or does this fall into the general cleanup and maintenance group of PR's? Thanks for putting up a PR, I think most of this makes sense minus the |
| for _, test := range tests { | ||
| t.Run(test.name, func(t *testing.T) { | ||
| _, _, err := ParseSQLMigration(strings.NewReader(test.sql), DirectionUp, false) | ||
| if err == nil || !test.errCheck(err) { |
There was a problem hiding this comment.
If we remove the GooseError, we can assert directly for the sentinel error with errors.Is ?
|
Hi @mfridman ! Thanks a lot for your review and feedback! Actually, this Pull Request was primarily for my self-assessment and a try to improve just one single part of the code in Open-Source project for better readability. My main goal was to gain a better understanding of the project's code and to test my own abilities. For the future contributing on this project - I will definitely focus on the section "Issues" before to provide PR. |
This Pull Request introduces the following improvements to the SQL parser:
GooseErrortype for better error handling.TestErrorHandlingfunction.These improvements aim to make it easier to understand and handle errors in the SQL parser, as well as improve the overall readability and maintainability of the code.