Error messaging context

[bradhanks]

I'm curious if it would be useful to add context to current error reasons.

For example, :closed in HTTP/2, specifying whether it was due to a connection state check or an unexpected closure during data transmission. I'm just reading through the code bc it's a foundational package and had that thought.

Also wondering if PR edits are welcome on stuff like inconsistent use of alias vs. full module name. Obviously not a big deal but I'm going through it so I thought I might as well ask if it's useful.

[whatyouhide]

Hey @bradhanks 👋

Also wondering if PR edits are welcome on stuff like inconsistent use of alias vs. full module name. Obviously not a big deal but I'm going through it so I thought I might as well ask if it's useful.

These are generally not very useful contributions, as usually there is a reason why we went with aliases vs full names. It's subjective anyways so hard to find a definitive best option 😉

As for context in errors, you're welcome to propose a concrete non-breaking API to add context, I'd love to see how that looks like.

[bradhanks]

One idea would be to delegate wrap_error/1 to /2 with an empty map and add:

%{file: ENV.file, fun: ENV.function, line: ENV.line}

as a second argument.

Then delegate format_error/1 and format_reason/1 to /2 and include the context in the message.

[whatyouhide]

%{
  file: __ENV__.file,
  fun: __ENV__.function,
  line: __ENV__.line
}

I don't see how this ☝️ is going to help. You'd need to dig into Mint's source code to understand why a connection was :closed. I was thinking more of something like adding context to :closed by having additional fields in Mint.HTTPError that can provide context on why an error happened, not just where?

[ericmj]

Seconding what @whatyouhide said. I think we can add additional context to determine the reason for a closed connection but I don't think adding source location metadata is the right way to do it.

A good error message should not require reading the source code to understand it. It also cannot be used to programmatically make decisions based on the error reason. I would rather add fields to Mint.HTTPError or make it a tuple for example: {:closed, :send_failed | :recv_failed | :remote_closed | :goaway | :invalid_frame}.

[bradhanks]

Seconding what @whatyouhide said. I think we can add additional context to determine the reason for a closed connection but I don't think adding source location metadata is the right way to do it.

A good error message should not require reading the source code to understand it. It also cannot be used to programmatically make decisions based on the error reason. I would rather add fields to Mint.HTTPError or make it a tuple for example: {:closed, :send_failed | :recv_failed | :remote_closed | :goaway | :invalid_frame}.

That makes a lot of sense. I think I had myself in mind as I was working through the different pathways. :)

I plan on going through it some more this week.

[whatyouhide]

A tuple could be seen as a breaking change though, so let's see if we can go with an additional field in the exception first 🙃

[bradhanks]

A tuple could be seen as a breaking change though, so let's see if we can go with an additional field in the exception first 🙃

open to any guidance and very much appreciated.