Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ArrayResponse and use it for GroupsClient.get_my_groups #575

Merged
merged 3 commits into from
Jun 1, 2022

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Jun 1, 2022

I noticed some code out in the wild doing

res = client.get_my_groups()
return {x["id"] for x in res.data}

Naturally, we'd like to be able to write this inline as

return {x["id"] for x in client.get_my_groups()}

but the response object is not iterable.

The overall goal of this PR is to enable the above style of usage by adding response.ArrayResponse and using that in GroupsClient.get_my_groups. ArrayResponse should support __iter__ and __getitem__ at least, arguably __len__ as well.

Along the way, I've checked for dunder methods other than __iter__ which are missing and which make sense to implement on the ArrayResponse type. Some interesting things:

  • __bool__ is reasonable in the normal cases, and even makes sense on dict data. You could take a very strict line and consider its introduction breaking because previously bool(response) was always True. I think that's far too strict, and have included __bool__
  • __getitem__ needs changes. At first, I thought ArrayResponse.__getitem__ would be specialized, but the base response class doesn't claim to not handle arrays. So __getitem__ now accepts str | int | slice in its annotation.
  • __len__ looks like it would work on the base response class, but it has some corner-case behaviors which would likely prove confusing. len(IterableResponse(...)) and len(response) when response.data is None are two of the more noteworthy. The first commit in this series defined __len__ on GlobusHTTPResponse, but it has been narrowed to only ArrayResponse.
  • some behaviors of __contains__, get() and other methods were untested

ArrayResponse is a new response type similar to IterableResponse which
defines `__iter__` on the raw `data` itself. As such, it benefits from
the addition of top-level `__bool__` and `__len__` so that it can be
tested and inspected similarly to any other Sequence type. It also
benefits from being able to use non-string keys (ints and slices) in
__getitem__.

Adding ArrayResponse therefore folds in these other improvements:
- `__bool__` which can now be false. A very strict reading could treat
  this as backwards incompatible for niche cases
- `__len__` is now defined (also works on dict data)
- `__getitem__` now takes `str | int | slice`, not just `str`
Test more corner cases of __repr__, __contains__, and get
Especially check on the handling of non-JSON data, which makes the
data attribute None
The meaning of len() on a generic response has ambiguities. On
ArrayResponse it is clear, so for now we will only include it there
and it will ensure that the data it receives is a sequence type
(meaning list).

The issues which make top-level __len__ less suitable for use:
- ambiguity of len() on IterableResponse objects (which len should it
  take?)
- limited usefulness of len(GlobusHTTPResponse) in general, now that
  __bool__ can be used as an empty check
- ambiguity of len() when data is None (0 or error?)
@sirosen sirosen merged commit ec549b1 into globus:main Jun 1, 2022
@sirosen sirosen deleted the add-array-response branch June 1, 2022 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant