steam icon indicating copy to clipboard operation
steam copied to clipboard

Upgrade SteamAuthenticatorError to have the eresult available where available.

Open luckydonald opened this issue 4 years ago • 4 comments

See #357.

luckydonald avatar Sep 28 '21 12:09 luckydonald

Hi not really much like to fake an API response just for the sake of having that default there. Like that Eresult has the meaning that it is coming from the API, and I would love to distinguish those errors not directly based on network traffic.

luckydonald avatar Sep 29 '21 10:09 luckydonald

I see it as extending and reusing the concepts established by Steam. Some of these errors are generated by the library itself and it doesn't make sense to have different set of errors given the Steam ones are already detailed enough. Generally, any failure in Steam results in a EResult.Fail, so it seem intuitive to have the as default for an exception with an option to specify a more appropriate value.

Additionally, it makes this change from minor enhancement to breaking change. Any code expecting to only get numerical value of eresult property now has to handle None.

rossengeorgiev avatar Sep 29 '21 21:09 rossengeorgiev

That is an issue, I see that. How about adding a separate EResult code, like -1 for that? I really want to separate library issues from api issues, otherwise we have not gained much.

Or I can simply drop the subclassing and then having None is no issue at all.

luckydonald avatar Sep 30 '21 23:09 luckydonald

  1. I don't see any benefit in having that separation
  2. The library doesn't do such separation elsewhere
  3. Fail is a generic catch all which good enough
  4. Introducing exceptions will introduce complexity for anything using the library. Special values like -1 carry risk if Steam decides to make use of them in the future

Any library specific errors can simply be handled defining more exceptions, which is the native way of doing it.

rossengeorgiev avatar Oct 02 '21 13:10 rossengeorgiev