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.

Sign up for GitHub

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

feat: adds base class definition for resources #83

Merged
merged 1 commit into from
Mar 13, 2024
+103 −2
Merged

feat: adds base class definition for resources #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/posit/connect/content.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@
from . import urls

from .config import Config
from .resources import Resources


class ContentItem(TypedDict, total=False):
# TODO: specify types
pass


class Content:
class Content(Resources[ContentItem]):
def __init__(self, config: Config, session: Session) -> None:
self.url = urls.append_path(config.url, "v1/content")
self.config = config
Expand All @@ -40,3 +41,12 @@ def get(self, id: str) -> ContentItem:
url = urls.append_path(self.url, id)
response = self.session.get(url)
return ContentItem(**response.json())

def create(self) -> ContentItem:
raise NotImplementedError()

def update(self) -> ContentItem:
raise NotImplementedError()

def delete(self) -> None:
raise NotImplementedError()
31 changes: 31 additions & 0 deletions src/posit/connect/resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from abc import ABC, abstractmethod
from typing import Generic, List, Optional, TypeVar


T = TypeVar("T")


class Resources(ABC, Generic[T]):
@abstractmethod
def create(self, *args, **kwargs) -> T:
raise NotImplementedError()

@abstractmethod
def delete(self, *args, **kwargs) -> None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does delete() make sense for these collections? Would this be a bulk delete action?

Copy link
Collaborator Author

@tdstein tdstein Mar 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 find(self, *args, **kwargs) -> List[T]:
raise NotImplementedError()

@abstractmethod
def find_one(self, *args, **kwargs) -> Optional[T]:
raise NotImplementedError()

@abstractmethod
def get(self, *args, **kwargs) -> T:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should id/guid be a part of the get() signature?

Copy link
Collaborator Author

@tdstein tdstein Mar 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? I assume that certain implementations may need to use a composite key for item references.

raise NotImplementedError()

@abstractmethod
def update(self, *args, **kwargs) -> T:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as delete: would this be for bulk updates (regardless of whether the APIs would force us to do one request per entity)?

raise NotImplementedError()
13 changes: 12 additions & 1 deletion src/posit/connect/users.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@

from requests import Session


from . import urls

from .config import Config
from .paginator import _MAX_PAGE_SIZE, Paginator
from .resources import Resources


class User(TypedDict, total=False):
Expand All @@ -25,7 +27,7 @@ class User(TypedDict, total=False):
locked: bool


class Users:
class Users(Resources[User]):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def __init__(self, config: Config, session: Session) -> None:
self.url = urls.append_path(config.url, "v1/users")
self.config = config
Expand Down Expand Up @@ -54,3 +56,12 @@ def get(self, id: str) -> User:
url = urls.append_path(self.url, id)
response = self.session.get(url)
return User(**response.json())

def create(self) -> User:
raise NotImplementedError()

def update(self) -> User:
raise NotImplementedError()

def delete(self) -> None:
raise NotImplementedError()
49 changes: 49 additions & 0 deletions tests/posit/connect/test_resources.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import pytest

from typing import Any, List, Optional

from posit.connect.resources import Resources


class TestResources(Resources[Any]):
def create(self) -> Any:
return super().create() # type: ignore [safe-super]

def delete(self) -> None:
return super().delete() # type: ignore [safe-super]

def find(self) -> List[Any]:
return super().find() # type: ignore [safe-super]

def find_one(self) -> Optional[Any]:
return super().find_one() # type: ignore [safe-super]

def get(self) -> Any:
return super().get() # type: ignore [safe-super]

def update(self) -> Any:
return super().update() # type: ignore [safe-super]

def test_create(self):
with pytest.raises(NotImplementedError):
self.create()

def test_delete(self):
with pytest.raises(NotImplementedError):
self.delete()

def test_find(self):
with pytest.raises(NotImplementedError):
self.find()

def test_find_one(self):
with pytest.raises(NotImplementedError):
self.find_one()

def test_get(self):
with pytest.raises(NotImplementedError):
self.get()

def test_update(self):
with pytest.raises(NotImplementedError):
self.update()
Loading