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.

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

refactor: Tag.update() should return None #369

Merged
merged 5 commits into from
Dec 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 4 additions & 4 deletions integration/tests/posit/connect/test_tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
27 changes: 25 additions & 2 deletions src/posit/connect/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,28 @@
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}
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)
2 changes: 1 addition & 1 deletion src/posit/connect/content.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
2 changes: 1 addition & 1 deletion src/posit/connect/env.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
7 changes: 2 additions & 5 deletions src/posit/connect/metrics/usage.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
9 changes: 4 additions & 5 deletions src/posit/connect/repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,19 +9,18 @@
runtime_checkable,
)

from ._utils import update_dict_values
from .errors import ClientError
from .resources import Resource, _Resource


# 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()
# # 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
Expand Down
2 changes: 1 addition & 1 deletion src/posit/connect/resources.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
11 changes: 6 additions & 5 deletions src/posit/connect/tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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`
def update(
self,
**kwargs,
) -> Tag:
) -> None:
"""
Update the tag.

Expand Down Expand Up @@ -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):
Expand Down
2 changes: 1 addition & 1 deletion tests/posit/connect/test_packages.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from posit.connect.client import Client

from .api import load_mock # type: ignore
from .api import load_mock


class TestPackagesFindBy:
Expand Down
11 changes: 4 additions & 7 deletions tests/posit/connect/test_tags.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Loading