Drop sync API for Activity, Notes, User Status, Weather Status#405
Open
Drop sync API for Activity, Notes, User Status, Weather Status#405
Conversation
Remove sync wrapper classes (_ActivityAPI, _NotesAPI, _UserStatusAPI, _WeatherStatusAPI) and their registrations in _NextcloudBasic. Async implementations remain unchanged. - Remove sync classes and NcSessionBasic imports from 4 module files - Remove sync type annotations, __init__ lines, and state resets from nextcloud.py - Remove all sync tests; async tests fully cover the same functionality - Fix talk_test async test to use AsyncNextcloud for user_status setup - Bump version to 0.30.0 - Add async-first deprecation notice to README
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughRemoves synchronous API classes for Activity, Notes, User Status, and Weather Status, making the library async-first; bumps version to 0.30.0 and adds README deprecation/migration notes. Tests were converted to async or removed to reflect async-only usage; some talk-bot helpers were made tolerant to missing fields. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
nc_py_api/activity.py (1)
186-188:⚠️ Potential issue | 🟡 MinorTypo in docstring: "avalaible" → "available".
📝 Proposed fix
async def get_filters(self) -> list[ActivityFilter]: - """Returns avalaible activity filters.""" + """Returns available activity filters.""" return [ActivityFilter(i) for i in await self._session.ocs("GET", self._ep_base + "/api/v2/activity/filters")]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nc_py_api/activity.py` around lines 186 - 188, Fix the typo in the docstring for get_filters: change "avalaible" to "available" in the triple-quoted string of the async def get_filters(self) -> list[ActivityFilter] method so the docstring reads "Returns available activity filters." (locate the get_filters method and its docstring near ActivityFilter and self._session usage).nc_py_api/notes.py (1)
77-78: 🧹 Nitpick | 🔵 Trivial
utcfromtimestampis deprecated since Python 3.12.Use
datetime.datetime.fromtimestamp(modified, tz=datetime.timezone.utc)instead for forwards compatibility.♻️ Proposed fix
`@property` def last_modified(self) -> datetime.datetime: """Last modified date/time of the note. If not provided on note creation or content update, the current time is used. """ modified = self._raw_data.get("modified", 0) - return datetime.datetime.utcfromtimestamp(modified).replace(tzinfo=datetime.timezone.utc) + return datetime.datetime.fromtimestamp(modified, tz=datetime.timezone.utc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nc_py_api/notes.py` around lines 77 - 78, The code in the method that reads self._raw_data ("modified = self._raw_data.get(\"modified\", 0)" followed by "datetime.datetime.utcfromtimestamp...") uses deprecated utcfromtimestamp; change it to use datetime.datetime.fromtimestamp(modified, tz=datetime.timezone.utc) so the method (where this appears — the accessor that returns the modified timestamp) remains forward compatible; ensure you import datetime.timezone usage if needed and keep the fallback default behavior for modified unchanged.tests/actual_tests/user_status_test.py (1)
21-30:⚠️ Potential issue | 🟡 MinorRemove assertion
status_icon is None—the property returns empty string by default.The
status_iconproperty (nc_py_api/user_status.py:66–68) returnsself._raw_data.get("icon", ""), which means it returns an empty string""when the icon key is absent. The return type isstr, neverNone. The assertion at line 23 of the test should beassert r1.status_icon == ""instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/actual_tests/user_status_test.py` around lines 21 - 30, The test _test_get_status incorrectly asserts r1.status_icon is None but the CurrentUserStatus.status_icon property returns an empty string by default; update the assertion in _test_get_status to assert r1.status_icon == "" so it matches the implementation of status_icon (which reads self._raw_data.get("icon", "") in CurrentUserStatus).nc_py_api/user_status.py (1)
198-203:⚠️ Potential issue | 🟠 MajorFix return type mismatch in
restore_backup_status.The method declares return type
CurrentUserStatus | Nonebut returns the raw dict from the OCS call. Other methods in this class consistently wrap results—e.g.,get_current()andget()both wrap withCurrentUserStatus(...)andUserStatus(...)respectively. Should bereturn CurrentUserStatus(result) if result else None.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nc_py_api/user_status.py` around lines 198 - 203, The restore_backup_status method returns the raw OCS dict but is annotated to return CurrentUserStatus | None; update restore_backup_status to wrap the OCS response in CurrentUserStatus (i.e., return CurrentUserStatus(result) if result else None) to match the pattern used by get_current() and get(), keeping the existing capability checks and the _session.ocs call intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nc_py_api/activity.py`:
- Around line 186-188: Fix the typo in the docstring for get_filters: change
"avalaible" to "available" in the triple-quoted string of the async def
get_filters(self) -> list[ActivityFilter] method so the docstring reads "Returns
available activity filters." (locate the get_filters method and its docstring
near ActivityFilter and self._session usage).
In `@nc_py_api/notes.py`:
- Around line 77-78: The code in the method that reads self._raw_data ("modified
= self._raw_data.get(\"modified\", 0)" followed by
"datetime.datetime.utcfromtimestamp...") uses deprecated utcfromtimestamp;
change it to use datetime.datetime.fromtimestamp(modified,
tz=datetime.timezone.utc) so the method (where this appears — the accessor that
returns the modified timestamp) remains forward compatible; ensure you import
datetime.timezone usage if needed and keep the fallback default behavior for
modified unchanged.
In `@nc_py_api/user_status.py`:
- Around line 198-203: The restore_backup_status method returns the raw OCS dict
but is annotated to return CurrentUserStatus | None; update
restore_backup_status to wrap the OCS response in CurrentUserStatus (i.e.,
return CurrentUserStatus(result) if result else None) to match the pattern used
by get_current() and get(), keeping the existing capability checks and the
_session.ocs call intact.
In `@tests/actual_tests/user_status_test.py`:
- Around line 21-30: The test _test_get_status incorrectly asserts
r1.status_icon is None but the CurrentUserStatus.status_icon property returns an
empty string by default; update the assertion in _test_get_status to assert
r1.status_icon == "" so it matches the implementation of status_icon (which
reads self._raw_data.get("icon", "") in CurrentUserStatus).
Contributor
Author
|
Pushed fix for flaky |
…ssages
The bot callback fires for all message types including system messages
(conversation created, bot joined, etc.) which lack object.content and
object.mediaType keys. This caused intermittent KeyError crashes in CI.
- Make object_content return {} when content key is missing
- Make object_media_type return "" when mediaType key is missing
- Skip bot handler assertions for non-user messages in test helpers
bigcat88
force-pushed
the
refactor/drop-sync-notes-weather-userstatus-activity
branch
from
c3bb50b to
19a8e27
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nc_py_api/talk_bot.py`:
- Around line 64-72: The method object_content() can return an empty dict but
ObjectContent currently requires "message" and "parameters"; update the
TypedDict declaration for ObjectContent to make those keys optional (use
total=False or mark keys as Optional) so the type matches possible empty dict
returns, then run the type checker; locate the ObjectContent TypedDict
definition and change it to a non-total TypedDict (or make both keys Optional)
to resolve the contract violation referenced by object_content().
Codecov Report❌ Patch coverage is
Additional details and impacted files
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
_ActivityAPI,_NotesAPI,_UserStatusAPI,_WeatherStatusAPI) from 4 module files_NextcloudBasic/NextcloudApptest_get_conversations_include_status_asyncin talk_test to useAsyncNextcloudinstead of syncNextcloudfor user_status setupMotivation
Moving nc_py_api toward async-only. This is the first batch of sync removals, targeting modules where async tests already mirror every sync test 1:1.
Test plan
grep -r '_NotesAPI\|_WeatherStatusAPI\|_UserStatusAPI\|_ActivityAPI'returns zero hitsSummary by CodeRabbit
New Features
Documentation
Refactor
Tests
Bug Fixes