Set SpecificFlowClient default scope object and adjust utils.classproperty #793
Conversation
The behavior of preferring an instance over a class, although valid and normal for a descriptor, is unlike classmethod and therefore may be surprising. However, it better allows us to define differentiated behaviors between instances and their classes, especially with respect to the `scopes` object on client classes. Without this change, it is not possible for a classproperty, when called, to refer back to the instance on which it was invoked. However, it is always possible to find the class via the instance, so expressive power is only increased by this change. Institute a convention of the classproperty decorated method accepting `self_or_cls` to clarify this at the usage sites. Comments on `classproperty` are updated. In particular, make sure that the current status of `classmethod(property(...))` stacking is reflected in those comments. A new test validates that the class attribute remains valid but the instance attribute is preferred when a classproperty is used to access an attribute.
`mypy` would flag the `scopes` object as `ScopeBuilder | None` even though the class always defined a de-facto value for this attribute on init. To resolve, insert a stub object as a class variable. Not only does this pacify mypy, but it also allows us to raise more useful errors when the classvar is accessed. Define the stub and several tests for it, including a typing test.
sirosen
requested review from
ada-globus,
derek-globus,
kurtmckee and
aaschaer
as code owners
There was a problem hiding this comment.
This is fine, but I have a feeling that the amount of code to support mypy, code to overcome mypy constraints and limitations, code to fix Sphinx documentation, comments that change linter behavior, and comments that explain why weird things exist, are -- taken together -- contorting the codebase with dubious payoff.
|
I tend to agree WRT Sans Maybe there's a more refined approach to doing this, but again, I think v4.0 would be a good time to think about it. Perhaps we can reshape the BaseClient behavior and expectations to better match |
pylint complains about an `__init__` not invoking `super().__init__`. If we do the "obvious" thing and call the superclass init with a placeholder value, attribute access patterns change. In particular, `self.resource_server` is assigned as an attribute, meaning that the concrete value there is discovered by the default `__getattribute__` before `__getattr__` can be invoked. To solve this, intercept that access with a custom `__getattribute__` to raise an error. The error will implicitly be handled and trigger a call of `__getattr__`, resulting in the desired `AttributeError` for `resource_server` access.
|
I had to push another change to pacify pylint, but I'm going to move forward with the merge based on the prior review. |
There are two changes here.
Primarily, this changeset drives at improving the behavior at typing time and runtime for users of SpecificFlowClient.
mypyshould allowscope: str = myclient.scopes.user-- for this, we need to manipulate thescopesclassvar a little.SpecificFlowClient.scopes.usershould still error, but with a useful message which points at instance-only usageIn service of these changes, it turns out that our internal classproperty descriptor needs a minor adjustment. Although it is only used to implement the
BaseClient.resource_serverclassproperty today, some pains are taken here to ensure that the behavior is kept unsurprising for future uses.📚 Documentation preview 📚: https://globus-sdk-python--793.org.readthedocs.build/en/793/