Uh oh!
There was an error while loading. Please reload this page.
feat!: remove all sync APIs (Phase 2) - #423
Draft
bigcat88 wants to merge 11 commits into
Draft
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
added 6 commits
April 26, 2026 10:50
BREAKING CHANGE: All synchronous API classes have been removed. The async classes are now the only implementation and have been renamed to drop the Async prefix: - AsyncNextcloud -> Nextcloud - AsyncNextcloudApp -> NextcloudApp - AsyncFilesAPI -> FilesAPI - AsyncTalkBot -> TalkBot - anc_app -> nc_app - atalk_bot_msg -> talk_bot_msg Backward compatibility aliases are provided and will be removed in v1.0.0. Removed ~2,950 lines of duplicated sync code.
- Convert test conftest to async (fixtures, setup/teardown) - Remove 174 sync test functions (duplicates of async tests) - Convert unique sync-only tests to async - Skip CalDAV tests (not available in async-only mode) - Convert test entry scripts (_install.py, _talk_bot.py, etc.) - Update README to reflect async-only API
The test conftest opens HTTP connections at import time via ``run_until_complete`` to check ``app_api`` capability and (later) NC version. After the sync removal in v0.31.0 those connections live in niquests AsyncSession pools bound to whatever event loop ran the ``run_until_complete`` call. pytest-asyncio runs the actual tests on a different loop, so the first request hits a stale pooled connection and fails with:: RuntimeError: got Future <Future pending> attached to a different loop Recreate the session adapters (``init_adapter(restart=True)`` and ``init_adapter_dav(restart=True)``) right after the import-time check so pytest's loop populates fresh empty pools lazily on first use. Apply the same reset after the version lookup in ``pytest_collection_modifyitems``, and short-circuit that hook entirely when no test carries the ``require_nc`` marker.
oleksandr-ncforce-pushed
the
remove-sync-apis
branch
from
April 26, 2026 10:51
9a22a76 to
65cbca3Compare
added 3 commits
April 26, 2026 11:18
…udApp After Phase 2 made ``NextcloudApp.log`` and ``NextcloudApp.set_init_status`` async, two ExApp helpers kept calling them as if they were sync — silently producing ``RuntimeWarning: coroutine '...' was never awaited`` and dropping the operation entirely. ``setup_nextcloud_logging`` is supposed to stream every Python log record into ``nextcloud.log``, and ``fetch_models_task`` is the default ``/init`` handler that reports model-download progress back to AppAPI; both were no-ops. logger.py * Capture the running event loop at ``setup_nextcloud_logging`` time and stash it on the handler. ``logging.Handler.emit`` is sync and can be called from any thread, so each record is dispatched to the captured loop with ``asyncio.run_coroutine_threadsafe``. Fire-and-forget, since logging must not block the caller. If the function is invoked outside a running loop (no-async setup), an inert handler is installed. integration_fastapi.py * ``fetch_models_task`` gains an optional ``loop`` parameter and is wrapped by a small ``_ProgressReporter`` that schedules ``set_init_status`` on the captured loop and waits with a 30s timeout for ordered progress updates. * The ``/init`` handler now captures ``asyncio.get_running_loop()`` and forwards it to the BackgroundTasks worker thread, where there is no loop of its own. * Inner ``__fetch_model_as_file`` / ``__fetch_model_as_snapshot`` accept the reporter callable in place of the bare ``NextcloudApp`` and use it for every progress tick. Bumps the version to 0.31.0.dev0 and adds a CHANGELOG entry covering the breaking sync removal, these regressions, and the conftest event-loop reset.
Phase 2 left a number of tests and examples calling the deprecated
``AsyncNextcloud`` / ``AsyncNextcloudApp`` / ``AsyncTalkBot`` / ``atalk_bot_msg``
aliases that now just point at the canonical async classes. The aliases stay
in place for users (and remain covered by ``tests/_install_async.py`` and
``tests/_install_only_enabled_handler_async.py`` which are the contract
tests for the backward-compat surface), but internal call sites should not
look like they need them.
* tests/actual_tests/misc_test.py — Async{Nextcloud,NextcloudApp} -> {Nextcloud,NextcloudApp}
* tests/actual_tests/talk_bot_test.py — AsyncTalkBot -> TalkBot
* examples/as_client/files/{listing,upload,find,download}.py — AsyncNextcloud -> Nextcloud
* examples/as_app/talk_bot{,_ai}/lib/main.py — atalk_bot_msg -> talk_bot_msg
Fix the ``examples/as_app/to_gif`` ExApp, which was a real regression: every
``nc.log`` / ``nc.files.*`` / ``nc.notifications.*`` / ``nc.ui.*`` call became
an unawaited coroutine after Phase 2. Convert ``convert_video_to_gif`` and
``enabled_handler`` to coroutines, await every call site, and offload the
CPU/IO-heavy ``cv2`` + ``imageio`` + ``pygifsicle`` pipeline to a sync helper
that runs via ``asyncio.to_thread`` so the event loop stays responsive.The previous fix piped every progress update through ``asyncio.run_coroutine_threadsafe(nc.set_init_status(...), loop)``. That works at runtime but breaks ``tests_unit/test_fetch_model_file.py``, which drives ``fetch_models_task`` from a regular sync test with a ``MagicMock`` ``nc``: there is no event loop and no real coroutine, so the loop-scheduling branch short-circuits and ``nc.set_init_status`` was never called at all. Make ``_ProgressReporter.__call__`` invoke ``nc.set_init_status`` unconditionally so mocks (and any future sync ``NextcloudApp`` shim) observe the call. Inspect the return value: if it is a coroutine, schedule it on the captured loop and wait for the result; if there is no loop available, ``coro.close()`` to avoid the "coroutine never awaited" warning; if it is not a coroutine at all (the mock case) just drop it. Also drop the empty ``error`` argument when calling ``set_init_status`` to keep the call signature identical to the pre-Phase-2 behaviour, so existing tests that assert ``call(100)`` rather than ``call(100, "")`` keep matching.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #423 +/- ##
==========================================
- Coverage 94.67% 94.06% -0.61%
==========================================
Files 48 48 Lines 5636 4585 -1051 ==========================================
- Hits 5336 4313 -1023 + Misses 300 272 -28
🚀 New features to boost your workflow:
|
added 2 commits
April 26, 2026 11:38
Add unit tests for the new code paths introduced when the logger handler and ``fetch_models_task`` were taught to schedule async :py:meth:`NextcloudApp.log` and :py:meth:`NextcloudApp.set_init_status` calls onto a captured event loop. These exercise the branches that the integration suite cannot reach: * ``_ProgressReporter`` with no captured loop (mock and real coroutine paths), with a closed loop, with errors raised by the scheduled coroutine, and the happy-path scheduling against a running loop via ``asyncio.to_thread``. * ``_NextcloudLogsHandler.emit`` short-circuiting when no loop / closed loop is attached, plus the dispatch path that observes the formatted record landing on the captured loop. * ``setup_nextcloud_logging`` capturing the running loop when called from async context and returning an inert handler when called outside one. * ``fetch_models_task`` accepting an explicit ``loop`` argument and falling back to ``asyncio.get_running_loop`` when invoked via ``to_thread``. Restores codecov coverage above the 1% drop threshold.
The sync removal is the project's first stable major release rather than a 0.31.0 minor — there is no remaining sync surface to refine, and the async-only API is the long-term contract. Move the version and changelog header to ``1.0.0`` accordingly, and drop the now self-referential "will be removed in v1.0.0" line about backward-compat aliases (they remain in this release for migration convenience and will be dropped in the next major).
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 freeto 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
Asyncprefix (AsyncNextcloud->Nextcloud, etc.)AsyncNextcloud = Nextcloud,AsyncNextcloudApp = NextcloudApp,AsyncTalkBot = TalkBot,anc_app = nc_app,atalk_bot_msg = talk_bot_msgBREAKING CHANGE: All sync APIs removed. Use async equivalents.
Test plan