-
Notifications
You must be signed in to change notification settings - Fork 28
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Better error formatting #132
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Introducing the preatty print was in my mind for quite some time.
To be honest I'm not sure if it wouldn't be easier just to rething the error message into something simpler by starting inspects from the beggining of the line:
** (Hammox.TypeMatchError)
Returned value does not match type
returned:
{:ok,
%Customers.Schemas.Customer{
__meta__: #Ecto.Schema.Metadata<:built, "customers">,
first_name: "John",
last_name: "Doe"
}, []}
type:
{:ok, Customers.Schemas.Customer.t(), [integer()]}
| {:ok, :no_customers}
| {:error, :not_found}
Value does not match type
retuned:
{:ok,
%Customers.Schemas.Customer{
__meta__: #Ecto.Schema.Metadata<:built, "customers">,
first_name: "John",
last_name: "Doe"
}, []}
type:
{:ok, Customers.Schemas.Customer.t(), [integer()]}
| {:ok, :no_customers}
| {:error, :not_found}
....
I believe that could be achieved with
human_reason
returning multi-line strings- original
left_pad
- and without the need of putting
padding
into process memory
I had this implemented but decided to not to modify the original messages for now. |
Thanks for the PR! The current Hammox messages are definitely quite unreadable and they do need work. Your proposal at a glance seems like a big readability improvement! However I'm thinking about what you mentioned:
I work in a big codebase and I definitely experience this. There are not only very long stack traces, but also many tests in the suite failing and printing them. In this situation, I'm not sure we'd want to enforce pretty printing, as it would make the output so much longer. Additionally, I think there is a deeper problem of displaying too much information in the first place. Currently the logic is pretty basic and we print whole structs for comparison. A better way to tackle the readability problem could be to be smarter about what is printed from the struct and decrease the total output. From this angle, pretty printing could potentially make the problem worse because it's easier to choose structs you want to dig into when they are on one line compared to them taking big chunks of screen space.
I think this could be a reasonable way forward. A |
Config option may be the way to go, it should not be hard to implement. I can look into it. |
3a5e480
to
69e2d39
Compare
I added config and some description |
Hey what's the status for this @msz? Could this PR be merged? |
This library is very usefull but I find the errors always so hard to read when it's inside a large struct because there are no newlines and when it's wrapped on 100 lines it looks like this (it's a simple example - this can be multiple screens long when refactoring legacy code):
I added some newlines and pretty printed the inspected structures - if it's a problem the inspect params can be set also in config.
Another useful feature would be different colors for data and messages.