Parse 'session_required_policies' to a list #678
Conversation
sirosen
requested review from
aaschaer,
ada-globus,
derek-globus and
kurtmckee
as code owners
There was a problem hiding this comment.
Looks good to me, part of me wants to somehow warn the user that there was session_required_policies, but it wasn't a string so something is wrong. But I can't think of a clean way to do that, and hopefully that issue would manifest in other ways like the session message not making sense.
I left a small style comment/question, but feel free to leave as is
src/globus_sdk/exc/err_info.py
Outdated
| # get str|None and parse as appropriate | ||
| session_required_policies = data.get("session_required_policies") | ||
| if isinstance(session_required_policies, str): | ||
| self.session_required_policies: t.Optional[ |
There was a problem hiding this comment.
Not a big deal, but it feels odd to only have the type information in the if branch and not the else branch for some reason. Does a pattern like this seem better or is it just me?
self.foo: t.Optional[t.fooType] = None
if condition_for_foo_existing:
self.foo = bar
There was a problem hiding this comment.
It's stopped looking odd to me to do the typing assignment like this, but that doesn't mean it isn't odd. It just means that I've done it too many times. 😉
I think I'll make a modification here to be similar to your suggestion, but also add a warning log for the present-but-not-string case. (I don't think we can do more than that.)
Rather than keeping the string which an API may pass back, convert it to the list of IDs by comma-splitting the value. If the value is not a string, the comma split won't be possible, so this is handled by treating anything other than a string as invalid (and setting the attribute to None). This also makes this property a little bit safer than many of our other computed properties, which are cast to the "right" types by the SDK but aren't validated. The change is technically backwards incompatible, but surrounds the use of a feature which is not in production use yet, and we have therefore agreed upon pragmatic treatment of this case as not requiring a major version bump. The test changes include updates to check the parsed value(s), but also a general update to the ErrorInfo tests to split a monolithic test into more individual cases.
If the type is not `str|None`, log a warning about the unexpected type.
sirosen
force-pushed
the
parse-required-policies
branch
from
7cf5ab9 to
e2433f4
Compare
Co-authored-by: Kurt McKee <contactme@kurtmckee.org>
Rather than keeping the string which an API may pass back, convert it to the list of IDs by comma-splitting the value. If the value is not a string, the comma split won't be possible, so this is handled by treating anything other than a string as invalid (and setting the attribute to None).
This also makes this property a little bit safer than many of our other computed properties, which are cast to the "right" types by the SDK but aren't validated.
The change is technically backwards incompatible, but surrounds the use of a feature which is not in production use yet, and we have therefore agreed upon pragmatic treatment of this case as not requiring a major version bump.
The test changes include updates to check the parsed value(s), but also a general update to the ErrorInfo tests to split a monolithic test into more individual cases.