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

Introduce an internal TypeGuard library, globus_sdk._guards #798

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

sirosen
Copy link
Member

@sirosen sirosen commented Aug 8, 2023

This is a very modest internal improvement, but it does lead to cleaner and clearer internal code which is slightly stricter in certain edge-cases.

Some of the improvements are more obvious than others. For example, here's a usage site which reads more easily now:

-        if isinstance(self._dict_data.get("errors"), list) and all(
-            isinstance(subdoc, dict) for subdoc in self._dict_data["errors"]
-        ):
+        if _guards.is_list_of(self._dict_data.get("errors"), dict):

Is this shorter / leaner / smaller? Not really.
Limiting ourselves to the src/ tree, _guards.py itself is 62 LOC, and we have +110,-68.
So if we discount the _guards.py module itself, it's marginally shorter. That's not really the point though.

This is more of a marginal improvement around the edges to how we do things, especially checking of list contents.
For example, in the TimerAPIError class, we have these changes:

-        elif isinstance(self._dict_data.get("detail"), list):
+        elif _guards.is_list_of(self._dict_data.get("detail"), dict):
 ... # context ...
-            self.errors = [
-                ErrorSubdocument(d)
-                for d in self._dict_data["detail"]
-                if isinstance(d, dict)
-            ]
+            self.errors = [ErrorSubdocument(d) for d in self._dict_data["detail"]]

These are not quite the same.
Because the only tool ready-to-hand before was isinstance(X, list), that was used, and then we had to handle, later, the contents of that list. Putting _guards.is_list_of(X, dict) in easy reach eliminates that potential point of divergence between various components.

In the first 2 commits, I went after the "easy" version of this. It works nicely.
In the second 2 commits, I went after an aspirational goal to make the GARE validators part of a broader suite of behaviors we have within the SDK. They add a bit of complexity (reduce / _Reducer), but I think the result is nice.


📚 Documentation preview 📚: https://globus-sdk-python--798.org.readthedocs.build/en/798/

This is a generic check for objects against `list[T]`.
e.g. `is_list_of(foo, str)`

It's added to a new module for type-guard-like helpers, `_guards`.
These are internal helpers for very common dispatching isinstance
checks.
Validators in the experimental GARE subpackage can be built from
TypeGuards if those type guards can be built in a form which is
suitable to some kind of chaining API in which the type is an input
and the guards are outputs or attributes.

`reduce` is a purpose-built helper in `_guards` which provides a
`_Reducer[T]` if given a `type[T]`.
`reduce(str) -> _Reducer[str]`.

The `_Reducer` can then have methods which act as guards of the
desirable signatures.

Finally, this can be pulled into the GARE component to write a
TypeGuard -> ValidatorCallback helper and wrap the results of `reduce`
calls.
This also leads to a minor change from __call__ to "apply()" in order
to ensure type-checkers read the types correctly.
@sirosen sirosen added the no-news-is-good-news This change does not require a news file label Aug 8, 2023
@sirosen
Copy link
Member Author

sirosen commented Aug 10, 2023

I've reverted and removed the fancy _Reducer. Per discussion, although it's cool, it's not intuitive enough to offset the cost of having to make sure that everyone understands it, or the potential maintenance burden if mypy or other type checkers start to treat it differently.

@sirosen sirosen merged commit f4e5391 into globus:main Aug 11, 2023
13 checks passed
@sirosen sirosen deleted the internal-guards branch August 11, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-news-is-good-news This change does not require a news file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants