fix: content overload signatures #290
Conversation
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
| @@ -23,7 +23,7 @@ def test_count(self): | |||
| assert self.client.content.count() == 1 | |||
|
|
|||
| def test_get(self): | |||
| assert self.client.content.get(self.content.guid) == self.content | |||
There was a problem hiding this comment.
I'm reaching the edge of my python knowledge here so maybe this is a dumb question but is .guid now a deprecated syntax and did that happen in this PR?
There was a problem hiding this comment.
It's self-imposed. See #262
We are moving away from attribute-based access for object data.
| body = dict(*args, **kwargs) | ||
| url = self.params.url + f"v1/content/{self.guid}" | ||
| response = self.params.session.patch(url, json=body) | ||
| def update(self, **attributes: Any) -> None: |
There was a problem hiding this comment.
ahh, I love this - definitely much easier to understand what's going on.
| response = self.params.session.patch(url, json=body) | ||
| def update(self, **attributes: Any) -> None: | ||
| """Update the content.""" | ||
| url = self.params.url + f"v1/content/{self['guid']}" |
There was a problem hiding this comment.
not sure how to weigh pros / cons here, just an observation - when I'm interacting with an SDK resource, my LSP would give me autocomplete on the deprecated property fields, which I don't get using the new approach. Its harder to explore what fields are available on the resource, or in some cases know where to look at all for that information if it isn't in a docstring.
Its also easier for the LSP to catch typos on properties - if I were to typo a dict key, I would get a more confusing error.
There was a problem hiding this comment.
Good callout. That is a big downside to this approach and the main reason we tried using attributes originally. I have experimented with defining a TypedDict and then holding data in an internal attribute instead of on self. I abandoned it due to the complexity of maintaining the facade between the internal state and self. I'm open to revisiting it, though.
The other answer is... you always have that problem when using dictionaries in Python. So this isn't anything new 🙃
There was a problem hiding this comment.
yeah, it might just be a matter of making sure we're good with our documentation of resources so we can easily check what's in them.
tdstein
force-pushed
the
tdstein/overrides
branch
from
d5bc486
to
b018dd9
Compare
tdstein
force-pushed
the
tdstein/overrides
branch
from
b018dd9
to
63c4851
Compare
Simplifies or, in most cases, corrects the overload signature. Variable names and documentation are updated along the way.
kwargsbased on context. The variable nameconditionsis introduced for querying methods. The variable nameattributesis introduced for record manipulation. This also signifies that this is not a passthrough method.Resolves #140