pretty format terms in errors #14269
Conversation
lib/elixir/lib/exception.ex
Outdated
| """ | ||
| no match of right hand side value: | ||
| #{inspect(exception.term, pretty: true, limit: :infinity)} |
There was a problem hiding this comment.
We can pretty print but we should not set it to infinity. Given this goes to production logs, it can have a pretty negative impact.
|
Good spot. |
|
I did a helper and used it in all places that use term in the message. The protocol error already formats the term with indentation so I just copied the pattern. |
| _ -> message <> "\n" <> format_stacktrace(stacktrace) | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
We should mark it as @doc false. Preferably underscore it too.
lib/elixir/lib/exception.ex
Outdated
| inspected = | ||
| term | ||
| |> inspect(pretty: true) | ||
| |> String.replace(~r/^(?=.+)/m, " ") |
There was a problem hiding this comment.
You should be able to achieve this by splitting on new lines and indenting every non empty string.
There was a problem hiding this comment.
We should add padding option to inspect at some point.
lib/elixir/lib/exception.ex
Outdated
| |> String.split("\n", trim: true) | ||
| |> Enum.map(&(" " <> &1)) | ||
| |> Enum.join("\n") |
There was a problem hiding this comment.
I don't think we want to trim, because that will remove empty lines and change what was formatted. You either want to indent all lines, which means a |> String.replace("\n", "\n ") or do:
| |> String.split("\n", trim: true) | |
| |> Enum.map(&(" " <> &1)) | |
| |> Enum.join("\n") | |
| |> String.split("\n") | |
| |> Enum.map(fn | |
| "" -> "" | |
| line -> " " <> line | |
| end) | |
| |> Enum.join("\n") |
| * :created_at | ||
| * :finished_at | ||
| * :started_at |
There was a problem hiding this comment.
This is also an issues:
I think it should be something like:
key :firts not found, did you mean:
* :first
in:
[ data: :structure]
...
This is more natural imo - you first check the suggestion and then go to the printed structure.
With the current implementation, you have to go back after reading the suggestion.
There was a problem hiding this comment.
Let's not change the order for now but we definitely need to fix the line breaking.
|
Ok, so I'm not sure how to handle the failing doctests, the rest is ready I think. |
|
Could we use #14210 in here? We may need to kill the doctests though. |
Yes, I knew that I saw it somewhere. It was implemented just in time for this. |
|
💚 💙 💜 💛 ❤️ |
It would be ideal if the exception would contain the left part of the match but just pretty printing it makes much difference.
I'm just not sure what to do with the doctest?