search

ValvePython / steam

☁️ Python package for interacting with Steam
http://steam.readthedocs.io
MIT License
1.06k stars 129 forks source link

Upgrade SteamAuthenticatorError to have the eresult available where available. #361

Open luckydonald opened 2 years ago

luckydonald commented 2 years ago

See #357.

luckydonald commented 2 years ago

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.

rossengeorgiev commented 2 years ago

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.

luckydonald commented 2 years ago

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.

rossengeorgiev commented 2 years ago
  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.