Add GlobusSDKUsageError & replace most ValueErrors #281
Conversation
There was a problem hiding this comment.
I'm fine with this idea. Obviously it should be mentioned in the release notes in case anyone has anything like try: . . . except GlobusError: . . . except ValueError: . . . in client code.
How about the ValueError that can be raised from BaseClient's __init__()?
| @@ -30,7 +31,8 @@ | |||
|
|
|||
| "GlobusResponse", "GlobusHTTPResponse", | |||
|
|
|||
| "GlobusError", "GlobusAPIError", "TransferAPIError", "SearchAPIError", | |||
| "GlobusError", "GlobusSDKUsageError", | |||
There was a problem hiding this comment.
Do you need to introduce this new error class in docs/exceptions.rst as well, including maybe rephrasing the "malformed calls to Globus SDK methods may raise standard python exceptions" bit?
There was a problem hiding this comment.
Yeah, I think a doc update would be appropriate. Will add.
| @@ -49,7 +51,7 @@ def endpoint_id(self): | |||
| if _on_windows(): | |||
| appdata = os.getenv("LOCALAPPDATA") | |||
| if appdata is None: | |||
| raise ValueError( | |||
| raise GlobusSDKUsageError( | |||
There was a problem hiding this comment.
Is the absence of LOCALAPPDATA on a Windows system the fault of usage, or is it the fault of a broken Windows installation? I don't think it matters, and we should probably leave it alone as it would be a breaking change, but maybe an OSError would have been better here.
There was a problem hiding this comment.
I was considering it a usage error from the start because it's not okay to try to use this method after clearing os.environ or invoking python with a really messed up env in the first place.
It's not possible for us to know why it wasn't set. So I just assume that it's the caller's fault at some level! 😄
I doubt this will ever get tripped, but #robustnessprinciple
Anyway, it sounds like you're okay with this error as it is in this PR.
|
The case of I don't want to consider this a breaking change, since cases that raised I think there's okay precedent across most libs for adding a new error type, which may break existing error handlers, as a minor release. |
Agreed; just a quick mention in release notes is fine. |
The GlobusSDKUsageError is a GlobusError which is also a ValueError. It replaces many uses of ValueError to indicate that an SDK method has been misused. This acts as an additional hint to users that they haven't read docs carefully enough, and makes us more conformant to our stated policy of only raising GlobusErrors unless otherwise noted. Include new exception in docs, and tweak text to mention its existence. Minor fix included: sphinx was warning and skipping on one of the flow manager objects -- fix the text on that page (which hopefully nobody is really reading and using...) to link correctly. Resolves globus#139
sirosen
force-pushed
the
feature/usage-error
branch
from
cd34b76 to
98f4a35
Compare
|
Rebased to include the |
The GlobusSDKUsageError is just
class GlobusSDKUsageError(GlobusError, ValueError): passIt replaces many uses of ValueError to indicate that an SDK method has been misused. This acts as an additional hint to users that they haven't read docs carefully enough, and makes us more conformant to our stated policy of only raising GlobusErrors unless otherwise noted.
That latter part (being consistent with our docs) is the one I care more about.
I left ValueError in place where it "felt appropriate", so feel free to call me out on that if any of it looks wrong.
Simple example:
now raises GlobusSDKUsageError, not ValueError, for passing authorizer to a confidential client.
Resolves #139