add async subject and source read methods to AsyncERClient by JoshuaVulcan · Pull Request #27 · PADAS/er-client

JoshuaVulcan · 2026-02-10T23:54:33Z

Summary

Adds get_subjects(), get_subject(), get_subject_tracks(), get_sources(), and get_source_by_id() to AsyncERClient, achieving feature parity with the sync ERClient for subject and source read operations.
get_sources() is implemented as a paginated async generator using the existing _get_data() infrastructure, consistent with get_events() and get_observations().
All methods mirror the parameter signatures of their sync counterparts (e.g., subject_group, include_inactive, date range for tracks).

Test Plan

31 new tests added covering:
- Successful responses for all 5 new methods
- Empty/no-result scenarios
- Query parameter passing and filtering (e.g., subject_group, include_inactive, since/until)
- Pagination across multiple pages (get_sources)
- HTTP error handling (401, 403, 404) raising appropriate ERClient* exceptions
- Network errors (ConnectTimeout, ReadTimeout) raising ERClientException
All 121 tests pass (90 existing + 31 new), zero regressions

Jira

Add get_subjects(), get_subject(), get_subject_tracks(), get_sources() (paginated async generator), and get_source_by_id() to AsyncERClient, achieving parity with the sync ERClient's existing read methods. Includes 31 new tests covering success, pagination, error handling, parameter filtering, and network failure scenarios. Resolves ERA-12652 Co-authored-by: Cursor <cursoragent@cursor.com>

Match PR #24 pattern so mocks match client request URL after #23. Add docs/AGENT_LEARNINGS.md for merge and test patterns. Co-authored-by: Cursor <cursoragent@cursor.com>

Copilot

Pull request overview

Adds async subject/source read APIs to AsyncERClient to match the sync client’s subject/source read capabilities, including paginated async iteration for sources.

Changes:

Added get_subjects(), get_subject(), get_subject_tracks(), get_sources(), and get_source_by_id() to AsyncERClient.
Added async tests covering success, empty results, query params, pagination, and error handling for the new endpoints.
Added an agent-focused documentation note capturing async test mocking patterns and merge guidance.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
`erclient/client.py`	Adds the new async subject/source read methods and wires `get_sources()` into existing `_get_data()` pagination/batching.
`tests/async_client/test_get_subjects.py`	New tests for `get_subjects()` and `get_subject()` (params + error cases).
`tests/async_client/test_get_subject_tracks.py`	New tests for `get_subject_tracks()` including date-range param behavior and errors.
`tests/async_client/test_get_sources.py`	New tests for paginated `get_sources()` and `get_source_by_id()` (success + error cases).
`docs/AGENT_LEARNINGS.md`	Adds internal notes about merge/test patterns and respx base URL usage.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

        """
        return await self._get(f"spatialfeaturegroup/{feature_group_id}", params={})

+    async def get_subjects(self, **kwargs):


+
+## 5. Strategy and merge order
+
+- Full merge strategy and wave order: see **ER_CLIENT_MERGE_STRATEGY_RECOMMENDATION.md** in the DAS repo (`/Users/joshuak/Projects/das/`) if available, or the same filename in notes.


+    async def get_sources(self, **kwargs):
+        """
+        Returns an async generator to iterate over sources (paginated).
+
+        Optional kwargs:
+            page_size: Change the page size. Default 100.
+            batch_size: The generator returns sources in batches (list)
+                        instead of one by one. Default 0 (means no batching).
+        """
+        page_size = kwargs.get('page_size', 100)
+        batch_size = kwargs.get('batch_size', 0)
+        params = {'page_size': page_size}
+        if batch_size and page_size:
+            params['page_size'] = batch_size
+        async for source in self._get_data(endpoint='sources', params=params, batch_size=batch_size):
+            yield source


+            page_size: Change the page size. Default 100.
+            batch_size: The generator returns sources in batches (list)
+                        instead of one by one. Default 0 (means no batching).
+        """
+        page_size = kwargs.get('page_size', 100)
+        batch_size = kwargs.get('batch_size', 0)
+        params = {'page_size': page_size}
+        if batch_size and page_size:
+            params['page_size'] = batch_size


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

+        if batch_size and page_size:
+            params['page_size'] = batch_size


+**Purpose:** Domain-specific context for agents and humans continuing the JoshuaVulcan PR merge work (main → open PR branches). Append to this file as new learnings arise.
+
+**Repo:** PADAS/er-client. **Base branch:** `main`. Use `gh` CLI (not GitHub MCP; org has SAML).
+


+- Full merge strategy and wave order: see **ER_CLIENT_MERGE_STRATEGY_RECOMMENDATION.md** in the DAS repo (`/Users/joshuak/Projects/das/`) if available, or the same filename in notes.
+- Merges are done by hand in the GitHub UI (merge commits). This doc and the learnings here support the person/agent doing the merge-main and conflict resolution on each branch.
+
+---
+
+## 6. Progress (merge main into PR branches)
+
+| PR | Branch | Status |
+|----|--------|--------|
+| 24 | ERA-12672/async-post-observation | Already up to date with main |
+| 25 | ERA-12658/v2-event-type-schema-updates | Merged main, resolved client.py, pushed |
+| 26 | ERA-12654/sync-get-event | Merged main, kept #26 get_event + main conftest, pushed |
+| 27 | ERA-12652/async-subject-source-read | Merged main; respx base_url fixed; **commit/push + next PR pending** |
+| 28–44 | (see strategy doc) | Pending |


+@pytest.mark.asyncio
+async def test_get_sources_unauthorized(er_client):
+    """Test get_sources raises ERClientBadCredentials on 401"""
+    async with respx.mock(
+        base_url=er_client._api_root("v1.0"), assert_all_called=False
+    ) as respx_mock:
+        route = respx_mock.get("sources")
+        route.return_value = httpx.Response(httpx.codes.UNAUTHORIZED, json={})
+
+        with pytest.raises(ERClientBadCredentials):
+            async for _ in er_client.get_sources():
+                pass
+
+        assert route.called
+        await er_client.close()
+
+
+@pytest.mark.asyncio
+async def test_get_sources_network_error(er_client):
+    """Test get_sources raises ERClientException on network error"""
+    async with respx.mock(
+        base_url=er_client._api_root("v1.0"), assert_all_called=False
+    ) as respx_mock:
+        route = respx_mock.get("sources")
+        route.side_effect = httpx.ConnectTimeout
+
+        with pytest.raises(ERClientException):
+            async for _ in er_client.get_sources():
+                pass
+
+        assert route.called
+        await er_client.close()


Copilot

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

+        page_size = kwargs.get('page_size', 100)
+        batch_size = kwargs.get('batch_size', 0)
+        params = {'page_size': page_size}
+        if batch_size and page_size:
+            params['page_size'] = batch_size
+        async for source in self._get_data(endpoint='sources', params=params, batch_size=batch_size):


+        Optional kwargs:
+            page_size: Change the page size. Default 100.
+            batch_size: The generator returns sources in batches (list)
+                        instead of one by one. Default 0 (means no batching).


+# Agent learnings: er-client merge and test patterns
+
+**Purpose:** Domain-specific context for agents and humans continuing the JoshuaVulcan PR merge work (main → open PR branches). Append to this file as new learnings arise.
+
+**Repo:** PADAS/er-client. **Base branch:** `main`. Use `gh` CLI (not GitHub MCP; org has SAML).
+
+---
+
+## 1. Project state (as of 2026-02-11)
+
+- **#23** (API versioning): merged. `_api_root(version)`, `base_url` params, `api_paths.py`, service_root normalization are on `main`.
+- **#45** (canonical async _delete + 204 in _call): merged. One-liner `_delete` and 204 handling are on `main`.
+- **#46** (shared sync_client conftest): merged. Canonical `tests/sync_client/conftest.py` and `__init__.py` are on `main`.
+- **#38**: closed. Use **#41** for analyzers/choices/gear/reports.
+- **#30**: merge **almost last**; coordinate with #23’s developer.
+
+---
+
+## 2. Respx / async test URL pattern (PR #24)
+
+After #23, the client builds request URLs with `_api_root(version)` (e.g. `https://example.org/api/v1.0/...`). Conftest’s `er_server_info["service_root"]` may still be the full URL including `/api/v1.0`.
+
+**Rule:** In async tests that use `respx.mock()`, set the mock **base_url** to the same base the client uses:
+
+- **Use:** `base_url=er_client._api_root("v1.0")`
+- **Do not use:** `base_url=er_client.service_root`
+
+Otherwise you get `RESPX: <Request('GET', '.../api/v1.0/subjects')> not mocked!` because the mock was registered against a different base.
+
+**Reference:** Latest commit on PR #24 (`ERA-12672/async-post-observation`), file `tests/async_client/test_post_observation.py`.
+
+**Files updated so far with this fix:**  
+`test_get_subjects.py`, `test_get_subject_tracks.py`, `test_get_sources.py` (PR 27).
+
+---
+
+## 3. Merging main into a PR branch
+
+1. `git fetch origin && git checkout <PR-branch> && git merge origin/main -m "Merge main to resolve conftest and infra conflicts"`.
+2. **Conflicts on conftest:**  
+   For `tests/sync_client/conftest.py` and `tests/sync_client/__init__.py`, keep **main’s** version:  
+   `git checkout --theirs -- tests/sync_client/conftest.py tests/sync_client/__init__.py` then `git add` those files.
+3. Resolve any other conflicts in `erclient/client.py` (or elsewhere) by combining main’s infra with the PR’s new methods; remove duplicate method definitions (e.g. keep one `get_event`).
+4. Run tests: `uv run pytest tests/sync_client/ tests/async_client/ -q --tb=line`. Fix any new failures (often respx `base_url` as above).
+5. Commit and push. If push is rejected (branch behind remote), `git pull origin <branch> --no-rebase`, resolve any new conftest conflict again with main’s version, commit, push.
+
+---
+
+## 4. Sync client `get_event` and tests
+
+- **#26** keeps a single `get_event(event_id, ...)` (positional) for both sync and async; main’s keyword-only `get_event(*, event_id=None, ...)` was removed to avoid duplicate.
+- Sync tests that assert exact URL equality with `er_client.service_root` can fail after #23. Prefer asserting that the path and `event_id` appear in the URL (e.g. `assert f"activity/event/{event_id}" in url`).
+
+---
+
+## 5. Strategy and merge order
+
+- Full merge strategy and wave order: see **ER_CLIENT_MERGE_STRATEGY_RECOMMENDATION.md** in the DAS repo (`/Users/joshuak/Projects/das/`) if available, or the same filename in notes.
+- Merges are done by hand in the GitHub UI (merge commits). This doc and the learnings here support the person/agent doing the merge-main and conflict resolution on each branch.
+
+---
+
+## 6. Progress (merge main into PR branches)
+
+| PR | Branch | Status |
+|----|--------|--------|
+| 24 | ERA-12672/async-post-observation | Already up to date with main |
+| 25 | ERA-12658/v2-event-type-schema-updates | Merged main, resolved client.py, pushed |
+| 26 | ERA-12654/sync-get-event | Merged main, kept #26 get_event + main conftest, pushed |
+| 27 | ERA-12652/async-subject-source-read | Merged main; respx base_url fixed; **commit/push + next PR pending** |
+| 28–44 | (see strategy doc) | Pending |
+
+---
+
+*Append new learnings below as you go.*


JoshuaVulcan requested review from chrisdoehring and chrisj-er February 10, 2026 23:54

JoshuaVulcan added autoreviewing PR is currently being auto-reviewed and removed autoreviewing PR is currently being auto-reviewed labels Feb 11, 2026

JoshuaVulcan and others added 2 commits February 11, 2026 17:35

Merge main to resolve conftest and infra conflicts

327fe06

fix: use _api_root('v1.0') for respx base_url in subject/source tests

8b6188a

Match PR #24 pattern so mocks match client request URL after #23. Add docs/AGENT_LEARNINGS.md for merge and test patterns. Co-authored-by: Cursor <cursoragent@cursor.com>

JoshuaVulcan requested a review from a team as a code owner February 12, 2026 01:38

Merge main into ERA-12652/async-subject-source-read

8f8c991

chrisdoehring requested a review from Copilot March 13, 2026 18:41

Copilot started reviewing on behalf of chrisdoehring March 13, 2026 18:42 View session

Copilot AI reviewed Mar 13, 2026

View reviewed changes

chrisdoehring closed this May 22, 2026

chrisdoehring reopened this May 22, 2026

chrisdoehring requested a review from Copilot May 22, 2026 21:53

Copilot started reviewing on behalf of chrisdoehring May 22, 2026 21:53 View session

Copilot AI reviewed May 22, 2026

View reviewed changes

chrisdoehring requested a review from Copilot May 26, 2026 20:26

Copilot started reviewing on behalf of chrisdoehring May 26, 2026 20:26 View session

Copilot AI reviewed May 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add async subject and source read methods to AsyncERClient#27

add async subject and source read methods to AsyncERClient#27
JoshuaVulcan wants to merge 4 commits into
mainfrom
ERA-12652/async-subject-source-read

JoshuaVulcan commented Feb 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants


		## 5. Strategy and merge order

		- Full merge strategy and wave order: see ER_CLIENT_MERGE_STRATEGY_RECOMMENDATION.md in the DAS repo (`/Users/joshuak/Projects/das/`) if available, or the same filename in notes.

		if batch_size and page_size:
		params['page_size'] = batch_size

		Purpose: Domain-specific context for agents and humans continuing the JoshuaVulcan PR merge work (main → open PR branches). Append to this file as new learnings arise.

		Repo: PADAS/er-client. Base branch: `main`. Use `gh` CLI (not GitHub MCP; org has SAML).

Conversation

JoshuaVulcan commented Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Jira

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JoshuaVulcan commented Feb 10, 2026 •

edited

Loading