Adds better error message #4794
Conversation
gampleman
force-pushed
the
jakub/fix/better-error-message
branch
from
733110d to
29914f3
Compare
|
Standard tests have passed 😊 |
There was a problem hiding this comment.
I am starting to wonder if this approach is somewhat flawed.
There are two problems here:
a) the line and column numbers should still exist close to where the error is described, like before. The hint is currently added somewhere in the middle. This makes the error uneccessarily hard to read.
b) the hint is too widely provided. Some of the test cases highlight how it is likely going to be more confusing than helpful, say in the case of having typed only SELECT. Quoting is going to no good in that case.
I think the problem is rather in the interaction of the tokenization and parsing. We shouldn't try to treat something that is at the position where a column is to be found as a core language feature token.
| assert error == "Expected `column definition` at line 1, column 7." | ||
|
|
||
| assert error == | ||
| "Expected a column definition.\n\n**Hint:** Maybe you need to add quotes around the definition? at line 1, column 7." |
There was a problem hiding this comment.
This has become a very awkward error message now. The line and column should be with the first sentence. I.e. the error should remain as is, with the hint appended for readability.
| @@ -1351,7 +1351,8 @@ defmodule Cloak.Sql.Compiler.Test do | |||
| views: %{"table_view" => %{sql: "select"}} | |||
| ) | |||
|
|
|||
| assert error == "Error in the view `table_view`: Expected `column definition` at line 1, column 7." | |||
| assert error == | |||
| "Error in the view `table_view`: Expected a column definition.\n\n**Hint:** Maybe you need to add quotes around the definition? at line 1, column 7." | |||
| @@ -1362,7 +1363,8 @@ defmodule Cloak.Sql.Compiler.Test do | |||
| views: %{"table_view" => %{sql: "select"}} | |||
| ) | |||
|
|
|||
| assert error == "Error in the view `table_view`: Expected `column definition` at line 1, column 7." | |||
| assert error == | |||
| "Error in the view `table_view`: Expected a column definition.\n\n**Hint:** Maybe you need to add quotes around the definition? at line 1, column 7." | |||
There was a problem hiding this comment.
In this case the hint is more misleading than it's useful. I am not sure we are on the right path here.
This can be fixed (albeit in a hacky way, although no more hacky than how we provide the line numbers in the first place).
That is why I phrased the error a hint with a question mark (i.e. intended as this potentially useful advice, but take it with a grain of salt). There are other situations (like So I see 2 ways forward:
|
|
I vote for option b). Let's abandon this pull-request, and think through how we can address this more fundamentally. But let's leave the more fundamental approach to a later time. I think it's complexity the sort of that can't make it into our current release, and furthermore complexity that we should think through 3 times whether the benefit it gives us does in fact warrant the complexity at all. |
Closes #4781 .