Miscellaneous refactoring

[bokner]

• Fix the bug with reading default options (which prevents, for instance, from using custom telemetry module)
• Unify context usage (it will represent what state FSM is at)
• Make unit tests compatible with OTP 26
• Replace deprecated Logger.warn/1 with Logger.warning/1

[starbelly]

what's the form of the term that requires an inspect here?

[bokner]

posix should be an atom, according to inet docs. I didn't give it much thought, just wanted to be consistent with what we have already done (line 206 of the current version).

[starbelly]

Yeah, I must have found that wasn't always the truth... should we just do an is_atom/1 perhaps? or the inverse with is_binary/1and/oris_list/1` ?

[bokner]

Not sure, I'd think as we return a formatted string that would probably be used predominantly in logs, why not to use inspect? This will also shield us from possible changes in upcoming OTP versions.

[starbelly]

That's fair, that said from a perf point of view inspect is going to be a little more expensive, not a concern but a thought, but I'm a bit confused here still. We're taking the a term, calling format error on it, which should guarantee a char list, converting it to string, checking to see if something is in the string, then doing an inspect on it, otherwise return it... am I missing something ? 😄

[bokner]

The confusion is probably caused by the fact that inet:format_error/1 works differently in OTP 25 and OTP 26

OTP 25:

iex(4)> :erlang.system_info(:otp_release)
'25'
iex(5)> :inet.format_error(:eh?)         
'unknown POSIX error'

OTP 26:

iex(4)> :erlang.system_info(:otp_release)
~c"26"
iex(5)> :inet.format_error(:eh?)
~c"unknown POSIX error: eh?"

, and we want to:

assert MLLP.Client.format_error(:eh?) == ":eh?"

So for the former, we need to use the original error, while for the latter we'd have to pluck it out of inet:format/1 output.
So we are kinda forced to use and format the original error to satisfy both versions.

Maybe the problem is that we consider everything that is not our standard error to be a POSIX error?
I'm thinking now that it could be not a bad idea to just remove this unit test :-)

[starbelly]

It might be yes! Sometimes we test too much :)

[bokner]

On the other hand, it could still be useful to keep this test case around. I'm not sure how we can improve on 'format_error/1'.

then doing an inspect on it, otherwise return it... am I missing something ?

We are doing inspect on the original error, not on the result of :inet.format_error/1. I added some comments that hopefully make it less confusing.

[starbelly]

Ahh. After some rest, it's tracking now. So in previous versions of OTP the atom would be tossed away and we would get back 'unknown POSIX error' every time, this has changed such that now we will get back a handy dandy char list that includes the string representation of the atom concatenated on the end. I think the code as it was, was fine in this regard, what you allude to is the problem. The test.

It's not really a great test. There's probably way too much specificity going on there. Really, I think the main thing we're interested in is "does this blow up?" or does it return something reasonable? If that's our interest then I think we should adjust the test to only look for the start of what we're expecting vs all of it.

But, if you think checking for exactness here is useful, I'm open to the change 😄

[bokner]

If that's our interest

Yes, I think we want to test if the non-POSIX 'atom' errors are accepted by 'format_error/1'.

then I think we should adjust the test to only look for the start of what we're expecting vs all of it.

Not sure I follow. We have two types of 'atom' errors: 1) POSIX, 2) non-POSIX. For the former, :inet.format_error/1 makes a human-readable message, which we want to use as is. For the latter, we ignore the output of :inet.format_error/1 and stringify it as is. The only purpose of start_with?/2 in the code is to detect whether the error is a POSIX one.