From f40afdabb9de1ec68ecdb50dbbb81142635eacbc Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 17 Dec 2024 11:29:06 -0500 Subject: [PATCH 1/5] Remove unused `drop_none()` --- src/posit/connect/_utils.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/posit/connect/_utils.py b/src/posit/connect/_utils.py index 26842bbc..9f61b2b8 100644 --- a/src/posit/connect/_utils.py +++ b/src/posit/connect/_utils.py @@ -3,5 +3,3 @@ from typing import Any -def drop_none(x: dict[str, Any]) -> dict[str, Any]: - return {k: v for k, v in x.items() if v is not None} From c8b17c2c8c0138e1226dd740255a97f75ba44e11 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 17 Dec 2024 11:31:07 -0500 Subject: [PATCH 2/5] Add `update_dict_values(obj, **kwargs)` helper method --- src/posit/connect/_utils.py | 25 +++++++++++++++++++++++++ src/posit/connect/repository.py | 7 +++---- 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/src/posit/connect/_utils.py b/src/posit/connect/_utils.py index 9f61b2b8..4b0ed291 100644 --- a/src/posit/connect/_utils.py +++ b/src/posit/connect/_utils.py @@ -3,3 +3,28 @@ from typing import Any +def update_dict_values(obj: dict[str, Any], /, **kwargs: Any) -> None: + """ + Update the values of a dictionary. + + This helper method exists as a workaround for the `dict.update` method. Sometimes, `super()` does not return the `dict` class and. If `super().update(**kwargs)` is called unintended behavior will occur. + + Therefore, this helper method exists to update the `dict`'s values. + + Parameters + ---------- + obj : dict[str, Any] + The object to update. + kwargs : Any + The key-value pairs to update the object with. + + See Also + -------- + * https://github.com/posit-dev/posit-sdk-py/pull/366#discussion_r1887845267 + """ + # Could also be performed with: + # for key, value in kwargs.items(): + # obj[key] = value + + # Use the `dict` class to explicity update the object in-place + dict.update(obj, **kwargs) diff --git a/src/posit/connect/repository.py b/src/posit/connect/repository.py index 1c562393..10c17045 100644 --- a/src/posit/connect/repository.py +++ b/src/posit/connect/repository.py @@ -9,6 +9,7 @@ runtime_checkable, ) +from ._utils import update_dict_values from .errors import ClientError from .resources import Resource, _Resource @@ -18,10 +19,8 @@ class _ContentItemRepository(_Resource): def update(self, **attributes): response = self._ctx.client.patch(self._path, json=attributes) result = response.json() - # # Calling this method will call `_Resource.update` which will try to PUT to the path. - # super().update(**result) - # Instead, update the dict directly. - dict.update(self, **result) + + update_dict_values(self, **result) @runtime_checkable From 12b7f63fa2df3d9972b27286d2d0857dea9569fd Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 17 Dec 2024 11:38:03 -0500 Subject: [PATCH 3/5] Make `Tag.update() -> None` instead of a new `Tag` --- integration/tests/posit/connect/test_tags.py | 8 ++++---- src/posit/connect/tags.py | 9 +++++---- tests/posit/connect/test_tags.py | 11 ++++------- 3 files changed, 13 insertions(+), 15 deletions(-) diff --git a/integration/tests/posit/connect/test_tags.py b/integration/tests/posit/connect/test_tags.py index 1d58263c..99e9deb4 100644 --- a/integration/tests/posit/connect/test_tags.py +++ b/integration/tests/posit/connect/test_tags.py @@ -151,12 +151,12 @@ def test_tag_content_items(self): } # Update tag - tagDName = tagD.update(name="tagD_updated") - assert tagDName["name"] == "tagD_updated" + tagD.update(name="tagD_updated") + assert tagD["name"] == "tagD_updated" assert self.client.tags.get(tagD["id"])["name"] == "tagD_updated" - tagDParent = tagDName.update(parent=tagB) - assert tagDParent["parent_id"] == tagB["id"] + tagD.update(parent=tagB) + assert tagD["parent_id"] == tagB["id"] assert self.client.tags.get(tagD["id"])["parent_id"] == tagB["id"] # Cleanup diff --git a/src/posit/connect/tags.py b/src/posit/connect/tags.py index 502a7b49..d109222f 100644 --- a/src/posit/connect/tags.py +++ b/src/posit/connect/tags.py @@ -5,6 +5,7 @@ from typing_extensions import NotRequired, TypedDict, Unpack +from ._utils import update_dict_values from .context import Context, ContextManager from .resources import Active @@ -162,14 +163,14 @@ def destroy(self) -> None: # Allow for every combination of `name` and (`parent` or `parent_id`) @overload - def update(self, /, *, name: str = ..., parent: Tag | None = ...) -> Tag: ... + def update(self, /, *, name: str = ..., parent: Tag | None = ...) -> None: ... @overload - def update(self, /, *, name: str = ..., parent_id: str | None = ...) -> Tag: ... + def update(self, /, *, name: str = ..., parent_id: str | None = ...) -> None: ... def update( # pyright: ignore[reportIncompatibleMethodOverride] ; This method returns `Tag`. Parent method returns `None` self, **kwargs, - ) -> Tag: + ) -> None: """ Update the tag. @@ -214,7 +215,7 @@ def update( # pyright: ignore[reportIncompatibleMethodOverride] ; This method r updated_kwargs = _update_parent_kwargs(kwargs) response = self._ctx.client.patch(self._path, json=updated_kwargs) result = response.json() - return Tag(self._ctx, self._path, **result) + update_dict_values(self, **result) class TagContentItems(ContextManager): diff --git a/tests/posit/connect/test_tags.py b/tests/posit/connect/test_tags.py index 9f4f7fb4..4bce01cd 100644 --- a/tests/posit/connect/test_tags.py +++ b/tests/posit/connect/test_tags.py @@ -289,20 +289,17 @@ def test_update(self): tag33 = client.tags.get("33") # invoke - updated_tag33_0 = tag33.update(name="academy-updated", parent_id=None) - updated_tag33_1 = tag33.update(name="academy-updated", parent=None) + tag33.update(name="academy-updated", parent_id=None) + tag33.update(name="academy-updated", parent=None) parent_tag = Tag(client._ctx, "/v1/tags/1", id="42", name="Parent") - updated_tag33_2 = tag33.update(name="academy-updated", parent=parent_tag) - updated_tag33_3 = tag33.update(name="academy-updated", parent_id=parent_tag["id"]) + tag33.update(name="academy-updated", parent=parent_tag) + tag33.update(name="academy-updated", parent_id=parent_tag["id"]) # assert assert mock_get_33_tag.call_count == 1 assert mock_update_33_tag.call_count == 4 - for tag in [updated_tag33_0, updated_tag33_1, updated_tag33_2, updated_tag33_3]: - assert isinstance(tag, Tag) - # Asserting updated values are deferred to integration testing # to avoid agreening with the mocked data From 030e9bd6e10ab2552b53d5d52341b531ef315361 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 17 Dec 2024 11:41:35 -0500 Subject: [PATCH 4/5] Be explicit on `.update()` methods returning `None` --- src/posit/connect/env.py | 2 +- src/posit/connect/repository.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/posit/connect/env.py b/src/posit/connect/env.py index 5ca26cd2..6656cd5a 100644 --- a/src/posit/connect/env.py +++ b/src/posit/connect/env.py @@ -131,7 +131,7 @@ def items(self): "Since environment variables may contain sensitive information, the values are not accessible outside of Connect.", ) - def update(self, other=(), /, **kwargs: Optional[str]): + def update(self, other=(), /, **kwargs: Optional[str]) -> None: """ Update environment variables. diff --git a/src/posit/connect/repository.py b/src/posit/connect/repository.py index 10c17045..2b81e554 100644 --- a/src/posit/connect/repository.py +++ b/src/posit/connect/repository.py @@ -16,7 +16,7 @@ # ContentItem Repository uses a PATCH method, not a PUT for updating. class _ContentItemRepository(_Resource): - def update(self, **attributes): + def update(self, **attributes) -> None: response = self._ctx.client.patch(self._path, json=attributes) result = response.json() From ef8497d357237f461404453a68664c2f5abcd389 Mon Sep 17 00:00:00 2001 From: Barret Schloerke Date: Tue, 17 Dec 2024 11:41:54 -0500 Subject: [PATCH 5/5] Remove type ignore statements --- src/posit/connect/content.py | 2 +- src/posit/connect/metrics/usage.py | 7 ++----- src/posit/connect/resources.py | 2 +- src/posit/connect/tags.py | 2 +- tests/posit/connect/test_packages.py | 2 +- 5 files changed, 6 insertions(+), 9 deletions(-) diff --git a/src/posit/connect/content.py b/src/posit/connect/content.py index 6adca5c3..5a8e7779 100644 --- a/src/posit/connect/content.py +++ b/src/posit/connect/content.py @@ -228,7 +228,7 @@ def restart(self) -> None: f"Restart not supported for this application mode: {self['app_mode']}. Did you need to use the 'render()' method instead? Note that some application modes do not support 'render()' or 'restart()'.", ) - def update( # type: ignore[reportIncompatibleMethodOverride] + def update( self, **attrs: Unpack[ContentItem._Attrs], ) -> None: diff --git a/src/posit/connect/metrics/usage.py b/src/posit/connect/metrics/usage.py index b9eac649..47f2f1f0 100644 --- a/src/posit/connect/metrics/usage.py +++ b/src/posit/connect/metrics/usage.py @@ -199,10 +199,7 @@ def find(self, **kwargs) -> List[UsageEvent]: for finder in finders: instance = finder(self._ctx) events.extend( - [ - UsageEvent.from_event(event) - for event in instance.find(**kwargs) # type: ignore[attr-defined] - ], + [UsageEvent.from_event(event) for event in instance.find(**kwargs)], ) return events @@ -252,7 +249,7 @@ def find_one(self, **kwargs) -> UsageEvent | None: finders = (visits.Visits, shiny_usage.ShinyUsage) for finder in finders: instance = finder(self._ctx) - event = instance.find_one(**kwargs) # type: ignore[attr-defined] + event = instance.find_one(**kwargs) if event: return UsageEvent.from_event(event) return None diff --git a/src/posit/connect/resources.py b/src/posit/connect/resources.py index 1916539d..cae3ab09 100644 --- a/src/posit/connect/resources.py +++ b/src/posit/connect/resources.py @@ -85,7 +85,7 @@ def __init__(self, ctx: Context, path: str, **attributes): def destroy(self) -> None: self._ctx.client.delete(self._path) - def update(self, **attributes): # type: ignore[reportIncompatibleMethodOverride] + def update(self, **attributes): # pyright: ignore[reportIncompatibleMethodOverride] response = self._ctx.client.put(self._path, json=attributes) result = response.json() super().update(**result) diff --git a/src/posit/connect/tags.py b/src/posit/connect/tags.py index d109222f..3144eecb 100644 --- a/src/posit/connect/tags.py +++ b/src/posit/connect/tags.py @@ -167,7 +167,7 @@ def update(self, /, *, name: str = ..., parent: Tag | None = ...) -> None: ... @overload def update(self, /, *, name: str = ..., parent_id: str | None = ...) -> None: ... - def update( # pyright: ignore[reportIncompatibleMethodOverride] ; This method returns `Tag`. Parent method returns `None` + def update( self, **kwargs, ) -> None: diff --git a/tests/posit/connect/test_packages.py b/tests/posit/connect/test_packages.py index 28ac75f4..2f1917a5 100644 --- a/tests/posit/connect/test_packages.py +++ b/tests/posit/connect/test_packages.py @@ -2,7 +2,7 @@ from posit.connect.client import Client -from .api import load_mock # type: ignore +from .api import load_mock class TestPackagesFindBy: