feat: adds base class definition for resources #83
Conversation
☂️ Python Coverage
Overall Coverage
New Files
Modified Files
|
tdstein
force-pushed
the
tdstein/resources-interface
branch
from
0a57800 to
b88bf12
Compare
| raise NotImplementedError() | ||
|
|
||
| @abstractmethod | ||
| def delete(self, *args, **kwargs) -> None: |
There was a problem hiding this comment.
Does delete() make sense for these collections? Would this be a bulk delete action?
There was a problem hiding this comment.
Based on this method definition, the implementation can do either. We haven't tried injecting the session object into the associated Resource yet. This should be solved as part of #23.
Typically, I would expect this method to act on a collection, which would either be the syntax sugar of user.delete() for user in users (pseudo-code) or a direct call to a DELETE endpoint that supports deleting multiple items in a collection.
| raise NotImplementedError() | ||
|
|
||
| @abstractmethod | ||
| def get(self, *args, **kwargs) -> T: |
There was a problem hiding this comment.
Should id/guid be a part of the get() signature?
There was a problem hiding this comment.
Maybe? I assume that certain implementations may need to use a composite key for item references.
| @@ -25,7 +27,7 @@ class User(TypedDict, total=False): | |||
| locked: bool | |||
|
|
|||
|
|
|||
| class Users: | |||
| class Users(Resources[User]): | |||
There was a problem hiding this comment.
What use is it to have Resources[User] vs. a simple Resources base class? Can/should you use the T type in the method definitions? Do you need to define the return types for the methods, or are they redundant to the base class?
More meta question: supposing you could factor some of those things out of the method definitions, should you? I could see an advantage of the current version being that it is explicit (famously better than implicit in Python ;), that you can read the class definition and see what it's doing.
There was a problem hiding this comment.
I'm not sure I follow. There may be something I need to learn about generics in Python.
Using a parametrized type in the ABC enforces that the inheriting class has consistent return types across methods. Are you suggesting that there are scenarios where this won't be the case?
There was a problem hiding this comment.
I guess one place I'm confused is that we're still defining the return types in this subclass (e.g. L38, find() returns List[User]). Can/should those type annotations be removed because the base class will handle them for you?
My other question is: for example, on L40, we call User(**user), can/should that be T(**user)?
The other way I've seen this done is that you would set self._resource_type = User or something and use that to construct the objects. (Not saying that's worth doing necessarily.)
There was a problem hiding this comment.
In this case, consider Resources an interface instead of an abstract class since it only has abstract methods. If Resources contained non-abstract methods, then T(**user) should be the correct usage for conversion in those methods (although it would be T(**result)).
I believe self._resource_type = User pattern was correct before generics were introduced in Python. This would have been the only way to inform the base class of the implementing type.
Can/should those type annotations be removed because the base class will handle them for you?
They can, but when writing the implementing code, the Python language server will auto-complete with the correct type definitions.
| raise NotImplementedError() | ||
|
|
||
| @abstractmethod | ||
| def update(self, *args, **kwargs) -> T: |
There was a problem hiding this comment.
Same question as delete: would this be for bulk updates (regardless of whether the APIs would force us to do one request per entity)?
Adding an ABC class to enforce the style choices defined in ./src/connect/README.md