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. |
|
💚 💙 💜 💛 ❤️ |
|
Hi there 👋 While testing Haven't yet dug too deep into this, and not saying there's an issue with that, just wanted to share it and double check that was the intention, given that the description in this pull request shows the output of the error in a test case failure only. So noting that this will also affect of how the exception message and banner is formatted in log output and any other packages that depend on Thanks! |
|
I personally noticed broken doctests in several packages that I maintain as well, and had to conditionally disable doctests in the CI depending on the version (example). The doctests also look less nice. I wonder if we could avoid the line breaks for smaller terms, by relying on inspect algebra and fitting rules 💭 |
|
It's a bit about what's better—human readability vs machine readability for tests. Maybe supporting multiline strings in iex somehow would be an option. Or having a way to use =~ in doctests? Most of the error messages generated by compiler now are multiline. |
We now have it through the use of ellipsis ( |
4 participants
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?