Describe the elements of Error_Protocol's three-tuple

[exarkun]

I was writing automated tests for some error handling cases in library that uses Network.TLS and found it unclear what all of the fields in this tuple (especially the Bool) meant. Here's an attempt at describing them.

[kazu-yamamoto]

I agree that this tuple is not well-documented.
What about replacing Bool with AlertLevel?

[exarkun]

I agree that this tuple is not well-documented. What about replacing Bool with AlertLevel?

That sounds reasonable to me. Happy to give it a try and see what happens.

[exarkun]

@kazu-yamamoto I pushed some more changes. I left the new haddock since it still seems more or less appropriate, but let me know what you think.

One thing I noticed while making this change is that there is exactly one use of Error_Protocol with AlertLevel_Warning against over a hundred uses with AlertLevel_Fatal. I wonder if it would make more sense to remove the AlertLevel from the Error_Protocol tuple entirely and replace the single AlertLevel_Warning use with a new TLSError constructor - Error_Warning or something. I haven't studied the implementation closely enough to know whether this is a quite simple change or if it would end up being more involved, though.

[kazu-yamamoto]

@exarkun I agree with you. Would you introduce Error_Protocol_Warning?
I think the Protocol name is important because this is a protocol level error resulting in sending an alert message.

[exarkun]

@kazu-yamamoto I added Error_Protocol_Warning and made the other necessary changes. I think it's ready for you to have another look. Thanks.

[kazu-yamamoto]

May I ask one more thing?

| Error_Protocol (String, AlertDescription)

I don't know why a tuple is used here. Would you try the following definition?

| Error_Protocol String AlertDescription

We should check Error_Protocol_Warning, too.

[exarkun]

May I ask one more thing?

| Error_Protocol (String, AlertDescription)

I don't know why a tuple is used here. Would you try the following definition?

| Error_Protocol String AlertDescription

We should check Error_Protocol_Warning, too.

I also don't know why these are tuples. This change seems reasonable (though, uggg, updating all those call sites again...).

[exarkun]

@kazu-yamamoto Pushed those changes (fwiw, I tried retrie for this one and it worked pretty well, doing almost all of the work for me).

[kazu-yamamoto]

The previous breaking change (#457) effected:

• http-client-tls
• http2-tls
• quic

But this breaking change effects only quic, which I can control.
I'm going to merge this PR.

[kazu-yamamoto]

Merged.
Thank you for your contribution!

How soon do you want to have version 1.9.0?
If you can wait, I play around this for about a week and then upload 1.9.0 on Hackage.

[exarkun]

Merged. Thank you for your contribution!

How soon do you want to have version 1.9.0? If you can wait, I play around this for about a week and then upload 1.9.0 on Hackage.

Thanks! I have no problem waiting - this was mostly a good excuse to try contributing a real (if minor) improvement to some project in my stack for the practice and to try to give some small bit back to the ecosystem. Thank you for guiding me through the process.

[kazu-yamamoto]

As you may know, a new version has been released.

[mbj]

at a low level at a low level.

Probably one low level to much ;)