feat: object-oriented rewrite — typed models with ActiveRecord behaviour#246
Draft
Olen wants to merge 42 commits into
Draft
feat: object-oriented rewrite — typed models with ActiveRecord behaviour#246Olen wants to merge 42 commits into
Olen wants to merge 42 commits into
Conversation
Adds DESIGN-oo-rewrite.md at the repo root capturing the design for the OO rewrite: ActiveRecord-style behaviour on typed objects, Pydantic v2 foundation, side-by-side deprecation, Person → Member/ Guardian split. Also bumps `pydantic = ">=2.0"` into runtime deps so subsequent commits on this branch can start implementing the typed models.
Adds the per-type Pydantic models that the OO rewrite returns from Spond.get_*(). No call-site changes yet — Spond still returns raw dicts; these classes are added in isolation so they can be wired up and tested incrementally. New files: - spond/_compat.py — DictCompatModel base with dict-style read access (subscript, get, contains, iter, keys/values/items, len). Emits DeprecationWarning from __getitem__ and .get() so existing dict-style callers get a one-time nudge per call site. - spond/event.py — Event (+ Responses, EventType StrEnum) with the three ActiveRecord methods: update(**fields), change_response( member_uid, *, accepted, decline_message=None), attendance_xlsx(). - spond/person.py — Person base + Member(Person) + Guardian(Person). Both flavours have send_message(text, group_uid) routed via the shared _send_message_to_person helper. - spond/group.py — Group with members/subgroups/roles as typed lists; find_member(*, uid|email|name) lookup helper; from_api() wires the client reference through to nested members + guardians. - spond/subgroup.py, spond/role.py — passive data classes. - spond/profile.py — Profile (the rich account record, distinct from the sparse `profile` reference dict on Member/Guardian). - spond/post.py — Post (Comment left as raw dict for now, future refinement). - spond/club.py — Transaction added. All classes use ConfigDict(populate_by_name=True, extra="ignore") so Spond API drift adds unknown fields silently rather than breaking validation. Live-probed against the real API to confirm field names and types.
…y writes Spond.get_profile/get_groups/get_group/get_person/get_events/get_event/ get_posts now return typed objects (Profile / list[Group] | None / Group / Person / list[Event] | None / Event / list[Post] | None). The caches on self.profile/groups/events/posts hold typed objects too. SpondClub.get_transactions returns list[Transaction]. Legacy write methods (Spond.update_event, change_response, get_event_attendance_xlsx) stay for backward compatibility but emit DeprecationWarning and delegate to the new ActiveRecord methods on the typed objects. _get_entity now uses .uid on cached typed objects instead of ["id"]. _match_person takes a Person and uses attribute access. A tolerant LenientDate validator was added to _compat.py because real Spond data occasionally has impossible dateOfBirth values (e.g. '2012-03-99'); strict ISO-8601 parsing would raise on those. Members and Profiles use it for date_of_birth. Existing tests adapted to construct typed objects via Event.from_api / Group.model_validate and assert via attribute access rather than dict-equality. 14 new tests added covering: DictCompatModel subscript + get + contains + iter behaviour, Event.update/change_response/ attendance_xlsx happy paths, Group → Member → Guardian materialisation, Group.find_member. 44 tests total, all green. Live-tested end-to-end against the real Spond API: 8 groups / 1405 members / 2 events / 2 posts parsed cleanly into typed objects, with the dict-compat shim emitting deprecation warnings on subscript access.
Updates the README "Example code" snippet to use attribute access (group.name, member.full_name) instead of dict subscripting, and adds a callout box explaining that v1.3 returns typed objects, that the dict-style fallback works with a DeprecationWarning, and pointing readers at DESIGN-oo-rewrite.md for the full design.
Contributor
There was a problem hiding this comment.
Pull request overview
Object-oriented rewrite that swaps the dict-based return surface of spond.Spond and spond.SpondClub for typed Pydantic models with ActiveRecord-style behaviour. Backward compatibility is preserved through a DictCompatModel shim that emulates dict access (with deprecation warnings) and through legacy method wrappers that delegate to the new typed methods.
Changes:
- New typed models:
Event/Responses/EventType,Group,Member/Guardian/Person,Subgroup,Role,Profile,Post,Transaction(all on a sharedDictCompatModelbase). Spond.get_*(andSpondClub.get_transactions) now return typed instances; legacy write methods (update_event,change_response,get_event_attendance_xlsx) emitDeprecationWarningand delegate to the newEvent.*methods.- Adds
pydantic >=2.0runtime dep, design doc, README example update, and tests for ActiveRecord methods, dict-compat shim, and group/member navigation.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| spond/_compat.py | New DictCompatModel base + LenientDate type alias backing dict-style read access. |
| spond/event.py | New Event model with update/change_response/attendance_xlsx. |
| spond/group.py | New Group model with find_member helper. |
| spond/person.py | New Person/Member/Guardian with send_message. |
| spond/post.py | New Post model. |
| spond/profile.py | New Profile model. |
| spond/role.py | New Role model. |
| spond/subgroup.py | New Subgroup model. |
| spond/club.py | Adds Transaction model; get_transactions returns typed list. |
| spond/spond.py | get_* return typed objects; legacy write methods become deprecated wrappers. |
| pyproject.toml | Adds pydantic >=2.0 runtime dependency. |
| README.md | Updates example to attribute-style access; adds typed-objects note. |
| DESIGN-oo-rewrite.md | New design doc describing the rewrite. |
| tests/test_spond.py | Adapts existing tests to typed objects; adds tests for OO surface, dict-compat, and navigation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
`Group.find_member` validated that exactly one search criterion was
supplied via:
supplied = [k for k in ("uid", "email", "name") if locals()[k] is not None]
This worked on Python 3.12+ because PEP 709 inlined comprehensions —
locals() inside the comprehension sees the enclosing function's scope.
On 3.11, comprehensions still have their own scope, so locals() inside
the comprehension is empty, and the indexed lookup raises KeyError.
Replaced with an explicit dict mapping that doesn't depend on the
comprehension's scoping behaviour. Cleaner anyway.
Six issues raised:
- Event.update() builds the POST payload via model_dump(by_alias=True),
but that emits native datetime objects which aiohttp's json.dumps
can't serialise — confirmed locally with a hard TypeError. Switched
to mode="json" so datetimes serialise as ISO strings. Same fix
applied to Spond.update_event's deprecation-wrapper round-trip.
- Event.type was annotated as the strict EventType enum, which would
ValidationError on any new event variant Spond introduces. Relaxed
to plain str; EventType kept as a constants reference for callers
who want to compare against known values.
- Spond.update_event / change_response / get_event_attendance_xlsx
deprecation wrappers now act as thin pass-throughs that preserve
the pre-OO byte-for-byte behaviour rather than delegating through
Event.* methods. This fixes:
* change_response: previously dropped any payload keys outside
{accepted, declineMessage} and defaulted accepted to "false" when
missing. Now forwards payload verbatim.
* update_event: previously raised ValueError on unknown keys (via
Event.update's strict _resolve_field_for_update). Now silently
ignores keys outside _EVENT_TEMPLATE, matching the old semantics.
Also re-added @_SpondBase.require_authentication so the deprecation
warning fires after auth, not before.
- Member.from_api / Guardian.from_api were dead code — Group.from_api
constructs them via Pydantic and walks them to set _client. Removed
the unreachable methods; updated the Guardian class docstring to
point at Group.from_api as the construction site.
- get_groups / get_events / get_posts were caching `[]` as `None`
when the API returned an empty list, changing cache semantics from
the pre-OO behaviour. Switched to `raw is None` check so empty
lists are cached as `[]` exactly as before.
Not actioned (verified false alarm):
- DictCompatModel.__iter__ yields keys (overriding Pydantic's tuple
iter), which the reviewer worried would break `dict(event)` and
`{**event}`. Live-tested: both work fine because Python's `dict()`
recognises the `.keys()` + `__getitem__` Mapping-like interface
and uses that path instead of __iter__. Left as-is.
44 tests pass; ruff clean; format clean.
Pre-OO, Spond.update_event used a hand-maintained `_EVENT_TEMPLATE` dict to define which fields the update payload should contain. That's exactly the kind of duplication the OO rewrite should eliminate: the Event class is now the schema of an event, and `Event.update()` builds its POST payload via `model_dump(by_alias=True, mode="json")` directly. Changes: - spond/event.py: Event.update() builds the payload from the instance's current state via model_dump (with mode="json" for datetime serialisation), overlays caller-supplied **fields, and POSTs. Unknown keys pass through to Spond verbatim rather than raising ValueError — Spond is the ultimate arbiter of what the API accepts, the SDK shouldn't gate. - spond/spond.py: Deprecated Spond.update_event wrapper now delegates straight to Event.update (no manual template merge). Same behaviour for callers: emit DeprecationWarning, fetch event, apply updates, POST. - spond/_event_template.py: DELETED. Was the technical-debt fixture this refactor exists to remove. - spond.py: dropped the `_EVENT_TEMPLATE: ClassVar` and the import. Note on field coverage: the old template included 4 fields the real-event GET response doesn't return — `spondType`, `maxAccepted`, `rsvpDate`, `payment`. Trusting Spond to fill in server-side defaults for these on update; if a user reports update breakage we can add them back as Event fields with explicit defaults. All 44 tests pass; ruff clean; format clean.
Eight of ten findings actioned: - Event required fields could crash on Spond API drift if a documented field was ever dropped. Made heading/start_time/end_time/ created_time/type/responses optional with sentinel defaults. Only `uid` stays required (every method addresses an event by id). __str__ guards against start_time=None. - Event.update() assumed Spond returns a full event on POST. If the response is partial (status-only, error wrapper) Pydantic would crash. Wrapped Event.from_api in a try/except ValidationError and fall back to a fresh `self._client.get_event(self.uid)` on failure. - Event.update(**fields) couldn't accept keys that clash with reserved kwargs like `self` / `cls`. Added a positional-only `_updates` dict parameter so callers with arbitrary keys can pass them as a dict. Spond.update_event wrapper now uses the positional form to avoid `**updates` collision risk entirely. - DictCompatModel.__len__ returned the count of fields declared on the class (~30 for Event), not the count of fields actually present in the source data. Switched to `self.model_fields_set` which mirrors the original dict's `len()` semantics. - _match_person could falsely match `None == None` if a member had no email on record and match_str was somehow None. Added an explicit bool() truthiness guard on person.email. - Group.from_api accepted `client: Spond | None` but propagated it unconditionally. Tightened the signature to `client: Spond` and documented that the SDK refuses to construct a no-client Group. - Stale comment in test_change_response — the wrapper is now a pure pass-through, so the test now asserts call args against `mock_payload` verbatim via `mock_put.assert_called_once_with(...)`. - Asymmetric `subGroups` (Member) vs `subGroupIds` (Post) aliases — Copilot suspected one was wrong; both match the live API. Added a docstring explaining the API itself is asymmetric. Not actioned (intentional design / non-issue): - `s.events` / `s.groups` etc. cache typed models, not dicts. Anyone serialising the cache via json.dumps directly would break. This is the intended OO direction; DictCompatModel provides the migration path. Not a regression to fix. - `Member.role_uids` typed as list[str] — Copilot worried it might sometimes be list[dict]. Verified against live data: it's list[str] for the account we tested. Leaving as-is; can revisit on reports. Added regression test `test_event_update_accepts_positional_dict` locking in the new dict form and unknown-key pass-through. 45 tests pass; ruff clean; format clean. Live-tested end-to-end.
Ten findings actioned (the eleventh — subtle _match_person behavioural nuance — was a no-op note, not a bug). DictCompatModel consistency (findings #1, #2): - __len__, __contains__, __iter__, keys, values, items now all use `_present_api_keys()` which returns the field names actually present in the source data. Pre-OO callers got `len(d) == len(list(d)) == sum(1 for _ in d)` on raw dicts; this restores that invariant. - New `_pydantic_extras()` helper surfaces fields preserved by `extra="allow"` (see below) so they participate in all dict-compat views, including __getitem__ (with its own targeted deprecation warning explaining the field is unmodelled). Event.update payload (findings #3, #4): - Added `_EVENT_READ_ONLY_FIELDS` whitelist of fields the update body should NOT carry: creator_uid, created_time, updated, expired, registered, modified_from_series, series_uid, series_ordinal, responses, recipients, comments. These are server-managed, derived, or have their own dedicated endpoints (e.g. change_response for responses). Critical: sending `responses` back risked Spond overwriting concurrent attendance changes with stale local state. - `Event.update()` now does `model_dump(exclude=_EVENT_READ_ONLY_FIELDS)` before overlaying caller updates, so the POST body matches the spirit of the pre-OO _EVENT_TEMPLATE whitelist without needing a separate template fixture. Forward compatibility with API drift (finding #7): - Switched every DictCompatModel subclass from `extra="ignore"` to `extra="allow"` (Profile, Member, Guardian, Group, Post, Subgroup, Role, Event, Responses, plus Transaction in club.py). Unknown fields Spond emits are now preserved on the instance and accessible both as attributes (Pydantic native) and via the dict-compat shim. Avoids silently dropping data on API drift. SpondClub.get_transactions atomicity (finding #6): - Build the page list before extending self.transactions, so a mid-page Pydantic validation failure can't leave the cache half-populated. Test fixes (findings #8, #9): - Removed misleading `s.events = [...]` from test_get_export — the deprecated wrapper doesn't consult the cache. - Restored the `assert result is not s.events` identity guard in test_update_event__returns_api_response — it's the explicit regression check for the original #239 bug. - Added `test_len_contains_and_iter_agree` locking in the new dict-compat consistency invariant, and `test_extra_allow_preserves_unmodelled_fields` covering the forward-compat behaviour. Documentation (findings #10, #11): - Event.update docstring tightened to make resolution bounds explicit (`bounded to Event.model_fields`). - README's "from v1.3 onwards" softened to "next minor release" since pyproject.toml still says 1.2.1. - DESIGN-oo-rewrite.md `#243` reference removed (rotting issue link). 47 tests pass; ruff clean; format clean. Live-tested end-to-end: real event has 29 present keys (matches API response), `len()` and iteration agree, the description field present in the API correctly shows in `in`/iter — the inconsistency between length and iteration is gone.
Six new findings, all actioned, plus proactive coverage. Member class cleanup (findings #1, #2): - Renamed `Member.fields` → `Member.custom_fields` with `alias="fields"`. The previous name collided with Pydantic's own `model_fields` vocabulary and risked confusion at call sites like `member.fields["height"]`. Live API serves it under the key `fields`; alias preserves the wire format. - Removed the redundant `model_config = ConfigDict(...)` redeclaration on `Member` — it duplicated `Person`'s config exactly and would have been a maintenance hazard if the two drifted out of sync. Pydantic v2 inherits `model_config` from the base. Event.update payload narrowed further (finding #3): - Added hidden, cancelled, match_event, behalf_of_uids to _EVENT_READ_ONLY_FIELDS, plus a doc comment grouping all excluded fields by reason (server-managed timestamps / derived flags / series wiring / nested sub-resources / not-in-pre-OO-template). The payload now matches the pre-OO template's writable set, so an `event.update(description="…")` no longer round-trips a stale local `cancelled=False` or `hidden=False` back over server state. Event.update cache refresh (finding #4): - After `Event.update()` returns the persisted Event, the client's `self.events` cache is updated in-place (index-based replacement preserving the list's identity for any caller holding a reference). Stops `spond.get_event(uid)` from returning the stale pre-update instance after an update. Defensive isinstance check on Person.profile (finding #5): - `_match_person` and `Spond.send_message`'s legacy path both used `person.profile is None or "id" not in person.profile`, which would raise `AttributeError` mid-scan if Spond ever returned a non-dict value for `profile`. Switched to `isinstance(person.profile, dict)` for parity with the rest of the lenient matcher. DESIGN-oo-rewrite.md / Comment status (finding #6): - Aligned the doc with the actual implementation: `Post.comments` is `list[dict[str, Any]]`, and a typed `Comment` class is explicitly deferred. Also reflected in the Files section. Proactive: docstring + test coverage: - Replaced `g["name"]` with `g.name` in the `Spond` class docstring's usage example — pre-OO dict-style would have emitted DeprecationWarning if a user copy-pasted it. - Added tests covering: extra="allow" on Profile and Group (not just Event); the new cache refresh after `Event.update()`; Member's custom_fields alias works via both Python name and API name. 50 tests pass; ruff clean; format clean. Live-tested: a real Member with `custom_fields` populated; Event.update payload now narrowed to 15 of 30 fields (matching the pre-OO template scope).
Five new findings actioned (one verified-as-incorrect, replied to): Stale-cache fallback in Event.update (finding #1): - The ValidationError fallback `await self._client.get_event(self.uid)` routes through `_get_entity`, which scans `self._client.events` first. At that point the cache still held the stale `self` (the cache replacement loop hadn't run yet), so the "fallback" returned the exact same stale event whose POST response had just failed to parse — a no-op. Reordered: invalidate `self._client.events = None` BEFORE the fallback fetch so it actually re-fetches from the API. The subsequent cache-replacement loop then writes the fresh event back (or no-ops harmlessly if `events` was None). Cosmetic Pydantic (finding #2): - `Field(default_factory=lambda: Responses())` → `default_factory=Responses`. Same default, less indirection. Test portability (finding #3): - The `extra="allow"` test asserted `p.newSpondField == 42` via native attribute access. Pydantic v2 supports this for `extra="allow"` but via `BaseModel.__getattr__` falling through to `__pydantic_extra__`, and behaviour varies subtly across minor versions especially for field names colliding with model_config-derived names. Switched the test to `p.__pydantic_extra__["..."]` for version-independence. Resource cleanup on all chat HTTP (finding #5): - `Spond._login_chat`, `Spond._continue_chat`, `Spond.send_message` (user/group_uid path), and `_send_message_to_person` in person.py all did `r = await session.post(...)` instead of `async with session.post(...) as r:`. The non-context-manager pattern doesn't release the response object back to the connection pool immediately and was inconsistent with every other HTTP call in the package. Wrapped all four. Updated the `test_send_message__continues _chat_when_chat_id_given` mock to use the standard `__aenter__` pattern that the rest of the test suite already uses. Live-tested the chat path (get_messages) after the rewrite — no resource warnings, response shape unchanged. Finding #4 (Member.email == email matching) — verified the existing guard in `Group.find_member` requires `email is not None` before the comparison, so the documented case is already safe. No action. Finding #6 (Spond.update_event wrapper field set) — Copilot's specific examples (`auto_reminder_type`, `participants_hidden`, `auto_accept`, `description`, `comments_disabled`) were all in the pre-OO `_EVENT_TEMPLATE`. Verified against the original `_event_template.py`. No action — will note in reply. Proactive sweep also caught: - No other `r = await session.post()` patterns elsewhere in the codebase. - No other `default_factory=lambda` patterns. - No other reliance on attribute-access for `__pydantic_extra__` keys. 50 tests pass; ruff clean; format clean.
Four items — three real bugs and one false flag.
## __aexit__: narrow the RuntimeError suppression (#3247599418)
`__aexit__` was wrapping `clientsession.close()` in
`contextlib.suppress(RuntimeError)` — way too broad. A future aiohttp
release raising RuntimeError for a real reason (connector failure,
resource leak) would be silently swallowed.
Replaced with `if not self.clientsession.closed: await close()`. The
defensive double-close case is handled cleanly; genuine errors now
propagate.
## validate_assignment=True on Event and Post (#3247599396)
`save()` uses `model_dump(exclude_unset=True)` to build the POST
payload. With Pydantic v2's default `validate_assignment=False`,
direct attribute assignment after construction (`post.title = "X"`)
does NOT update `__pydantic_fields_set__`. So mutating a previously-
unset field and then calling `save()` silently dropped the change.
Enabling `validate_assignment=True` on Event and Post fixes this
cleanly — assignments now route through Pydantic's validator chain
which both validates the new value AND records the field as set.
Regression test added in `test_event_save_delete.py::TestSaveUpdate
::test_save_persists_mutation_of_unset_field`.
## __eq__/__hash__ invariant (#3247599307)
The previous fallback was `BaseModel.__eq__` (full-field equality)
for `__eq__` paired with `object.__hash__` (identity-based) for
`__hash__`. Two distinct instances with identical state were equal
but had different hashes — violating Python's
`a == b ⟹ hash(a) == hash(b)` invariant.
Adopted a two-tier model:
- **Entity types** (have a natural key): equal iff keys match;
hashable via the same key. Match/Event and Member/Guardian still
compare equal across classes via their shared entity-kind tag in
the key tuple.
- **Value types / sub-objects** (no natural key — Responses,
MatchInfo): equal iff every declared field matches (still useful
for `model_equals()` propagating through nested fields), but
`__hash__` raises `TypeError("unhashable type")` — same convention
as `dict` / `list` / `set`. Their declared fields contain mutable
lists and dicts; making them hashable would require hashing those
too, which doesn't work.
## Test renames — false flag (#3247599368)
Copilot saw `test_save_create_appends_to_client_cache` (Event) and
`test_save_create_appends_to_cache` (Post) and inferred from the
NAMES that the two methods used different ordering. Both actually
use `insert(0, ...)` (prepend) — the test names were misleading.
Renamed both to `test_save_create_prepends_to_cache` and added an
explicit "new at position 0, existing slid down" assertion using a
pre-populated cache so the prepend behavior is now self-evident from
the test body.
Live API re-verified end-to-end: event create + mutate-then-save +
delete + post create/delete all still 200 in the test group.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Inconsistency between Event.save() and Post.save() spotted in review: Post cached `self` after the in-place mutation, so callers could rely on `post is spond.posts[0]` after a successful save. Event, however, cached `refreshed` (the new instance built from the API response) *before* mutating self, so `event is not spond.events[0]` even though their field state matched. The two ActiveRecord types should agree. Aligned Event.save() to Post's contract: - Create path: insert `self` at position 0, not `refreshed`. The in-place mutation of self has already brought it up to date with the persisted state. - Update path: after `await self.update()` (which writes its own returned instance into the cache), overwrite that slot with `self` so the identity guarantee holds on this path too. The choice between caching self vs caching refreshed is mostly philosophical, but ActiveRecord's premise is "self is the live record" — caching anything else creates two objects with identical state and no way for the caller to tell them apart, which breaks set/dict-key semantics (and makes future "did anyone modify this since save?" introspection awkward). Tests added on both Post and Event: - `test_save_create_caches_self_not_refreshed_copy` (both files) - `test_save_update_caches_self` (Event only, since this is the path that delegates through update() and required extra fix-up) Live API re-verified: after `event.save()` (create AND mutate-then-save), `event is s.events[0]` evaluates True in both cases. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ideas borrowed from elliot-100/Spond-classes — the read-only
typed-wrapper library that informed parts of this rewrite. Two
additions that materially improve OO ergonomics:
## Navigation helpers on Group
Five synchronous, no-HTTP helpers that encode common membership-graph
queries every caller would otherwise write inline:
```python
member = group.member_by_uid(uid) # shorthand for find_member(uid=...)
role = group.role_by_uid(uid)
subgroup = group.subgroup_by_uid(uid)
coaches = group.members_by_role(role) # or by uid string
team_a = group.members_by_subgroup(sg) # or by uid string
```
`members_by_*` accept either a typed Subgroup/Role instance OR its
uid string — typed for callers walking `group.subgroups`,
string-accepting for callers holding only a uid (e.g. from
`member.role_uids`).
## Typed FieldDef
Spond Groups expose `fieldDefs` — definitions for the custom-data
slots members can fill (shirt size, emergency contact, etc.).
Previously `Group.field_defs: list[Any]` (raw); now
`list[FieldDef]` with `uid` + `name`.
This makes the previously-awkward "join group's field labels with
each member's `custom_fields` values" pattern trivial:
```python
for fd in group.field_defs:
print(f"{fd.name}: {member.custom_fields.get(fd.uid)}")
```
Only `uid` is strictly required on `FieldDef`; `extra="allow"`
preserves anything Spond adds in future releases (type, ordering,
required flags) without breaking validation.
## What was considered but not adopted
Several other ideas in Spond-classes were evaluated and skipped for
specific reasons documented in the PR thread:
- `Member.email: EmailStr` — adds `email-validator` dep, rejects
malformed emails; we prefer permissive pass-through
- `Event.type: Literal[...]` — constrains at type level; we
intentionally use `str` + a separate `EventType` enum for
forward-compat against new Spond variants
- `Event.is_cancelled` / `is_hidden` properties — trivial wrappers
around the bool fields; bikeshed
- `list_from_data` classmethods — cleaner but adds API surface for
marginal benefit
- `validate_list_of_data_dicts` shape check (feat/check-responses
branch) — Pydantic already does this at model_validate time with
clearer errors
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ost update
Six review items resolved, including two real bugs found during live
re-verification.
## Real bugs
### 1. Wrong HTTP verb for Post update
Post.save() update path used POST /posts/{uid}. Spond returns 405
Method Not Allowed; the right verb is PUT. Verified live in the test
group. Update path now uses `clientsession.put`; create still uses
POST `/posts/`. Inconsistent with Event (which uses POST for both
verbs) but matches Spond's API contract.
The previous tests passed because they mocked
`aiohttp.ClientSession.post` for both code paths — the bug was
invisible to mocks. Tests now mock `put` for the update path. The
TestPostRoundtrip lifecycle test splits its mocks across post/put
explicitly.
### 2. save() wiped locally-appended comments (and reactions/responses)
`save()`'s in-place state copy iterated every model field and
overwrote self with refreshed. On UPDATE, Spond's response often
omits `comments` (and `reactions`) — those have their own dedicated
endpoints. A caller doing:
await post.add_comment("hi") # post.comments = [Comment("hi")]
post.title = "Renamed"
await post.save() # would wipe post.comments
would silently lose the just-added comment.
Fix: skip `comments`/`reactions` (Post) and `comments`/`responses`
(Event) on the UPDATE path of the in-place state copy. The CREATE
path still applies them — Spond's create response IS the canonical
fresh state for a brand-new entity.
Regression tests in
test_post_save_delete.py::TestSaveDoesNotWipeLocallyAddedComments
cover both the "response omits the key entirely" and "response
returns an empty list" cases, plus a positive control that create
still applies the response's comments.
## Other items
### 3. tests/test_groups.py — misleading AsyncMock comment
`s.get_groups = AsyncMock() # leaves self.groups as None` was
incorrect — AsyncMock returns a MagicMock instance when awaited,
not None. Replaced with `AsyncMock(return_value=None)` and an
accurate inline comment.
### 4. spond/post.py — document deliberate no-_client on Comment
Added a comment at the `Comment.model_validate(data)` site in
`add_comment` explaining that comments carry no ActiveRecord ops of
their own (Spond exposes no edit/delete endpoints for individual
comments via the consumer API), so the new instance doesn't need
client wiring. Flag for future expansion.
### 5. spond/comment.py — line length
Split the long `Comment.__str__` return into a multi-line f-string.
### 6. tests/test_context_manager.py — explicit spy on double-close
Strengthened `test_double_close_does_not_raise` to install an
`AsyncMock` spy on `clientsession.close` after the manual close
inside the `with` block, then assert `spy.await_count == 0` to
verify __aexit__'s closed-check actually short-circuits rather than
just tolerating a second call.
Live API re-verified end-to-end: create + add_comment + mutate +
save (PUT) + delete; comments survive the save.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## examples/ Every example now uses the typed-object surface — attribute access instead of dict subscripts, `async with Spond(...)` for automatic session cleanup, and per-type model methods (e.g. `event.attendance_xlsx()` instead of the deprecated `Spond.get_event_attendance_xlsx(uid)` wrapper). - `ical.py` — typed `event.heading` / `event.meetup_time` etc.; added `Event.meetup_time` field so the canonical "use meetup time if present, else kickoff" pattern reads cleanly without dict fallback. Migrated to `async with`. - `groups.py` — uses `group.model_dump_json(by_alias=True, indent=4)` for the per-group JSON dump. The pre-OO version did `json.dumps(group)` which now fails because a typed Group isn't JSON-serialisable by default; the Pydantic serialiser does the right thing. - `attendance.py` — typed `event.responses.accepted_uids` etc. instead of `event["responses"]["acceptedIds"]`; uses `person.full_name` from the typed Member/Guardian return of `get_person()`. - `transactions.py` — typed `Transaction` instances dumped via `model_dump(by_alias=True, mode="json")` for the CSV rows. - `manual_test_functions.py` — every `_*_summary()` helper now reads typed attributes; switched to `async with`; uses `events[0].attendance_xlsx()` (the OO method) for the export demo so users see the recommended shape. ## README.md New section "⚠️ Upgrading to v2.0 — read this first" near the top covers: 1. The semantic changes callers need to audit before upgrading (equality, return types, exception classes, deprecated wrappers). 2. How to **pin to `< 2.0.0`** to defer the upgrade (pip, pyproject.toml, requirements.txt syntaxes). 3. How to find every dict-style access site in their code by running with `-W error::DeprecationWarning`. ## spond/event.py Added `Event.meetup_time` (alias `meetupTimestamp`) — Spond's "oppmøtetid" field used by match events for arrival time. Previously this lived in `__pydantic_extra__` and was only reachable via the dict-shim. The ical example demonstrates the canonical `event.meetup_time or event.start_time` fallback shape. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## SpondAPIError.__str__ always includes URL when present
Previously the URL was only appended on the no-body path
(`if url and not body`). For the more common diagnostic case
(`SpondAPIError(503, "Service Unavailable", "https://...")`), the
URL was silently dropped from the message — the worst of both worlds,
since the body+url combination is the most useful one to a developer
reading a traceback. Now the URL is always appended when set,
regardless of whether there's a body.
## Missing `.ok = True` in three Event save tests
Three tests in `tests/test_event_save_delete.py` mocked
`aiohttp.ClientSession.post`'s `.json` but not `.ok`. They passed
because `MagicMock().ok` evaluates truthy by default — the `if not
r.ok:` check in production happened to pass by accident. If the
production code is ever tightened to `if r.ok is not True:`, the
tests would silently break. Set `.ok = True` explicitly to match
the convention used in the rest of the file.
## `Person.__str__` / `Profile.__str__` show `<unnamed>` for empty names
`full_name` returns `""` when both first/last name default to empty
strings (per the resilience relaxation). `Person.__str__` then
rendered as `Member(uid='M1', name='')` — the empty quotes look like
a bug to a reader. Now shows `name='<unnamed>'`, mirroring the `"?"`
sentinel pattern already used by Event/Post/Comment for missing
timestamps. Pure cosmetic change to debug output.
## Boilerplate-fixture suggestion (declined, optional)
The fourth comment proposed extracting a helper fixture in
conftest.py to centralise the duplicated `mock_post.return_value
.__aenter__.return_value.{ok,json}` pattern across ~20 tests. Genuine
maintainability improvement, but a separate change — touching every
test file at once would dwarf this round's fixes and obscure the
substantive ones. Leaving as a follow-up if anyone wants to take it
on.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…r nit ## Replace __pydantic_extra__ wholesale on save() (real bug) `save()` was merging refreshed extras into self via `.update()` while replacing `__pydantic_fields_set__` wholesale on the next line — producing an inconsistent picture where dict-compat iteration (`list(post)`, `post.keys()`) would report stale extras that the refreshed response didn't include. Now `__pydantic_extra__` is `.clear()`-then-`.update()`-d to match the wholesale replacement semantics used for declared fields. Same fix in both `Event.save()` and `Post.save()`. Regression test in `test_post_save_delete.py::TestSaveExtrasReplaceNotMerge` covers both directions: - A pre-existing extra absent from the response is gone after save - An extra in the response is present after save Live-verified on the test group: seeded a stale extra on a post before save, mutated title, called save() — the stale extra is no longer on the post after the round-trip. ## `__aenter__` return-type annotation Added `-> Self` (and the `from typing import Self`, `from __future__ import annotations`) so `async with Spond(...) as s:` type-checkers see `s: Spond` directly without falling back to inference. Consistent with the rest of the file. ## Grammar nit in examples/transactions.py "Creates an transactions.csv" → "Creates a transactions.csv". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Same pattern as the previous round's #3249591939 fix: three tests mocked `aiohttp.ClientSession.post`'s `.json` but not `.ok`, passing only because `MagicMock().ok` evaluates truthy by default. Set `.ok = True` explicitly to match the convention used in the rest of the suite and make these tests robust against any future tightening of the production `if not r.ok:` check. Tests fixed: - `tests/test_backward_compat.py::test_update_event_emits_deprecation` - `tests/test_compat.py::test_event_update_excludes_unset_default_collections` - `tests/test_compat.py::test_event_update_excludes_none_fields` The third review item (ical.py field name sanity check) verified clean: `Event.meetup_time`, `Event.updated`, and `Event.cancelled` are all declared fields with the correct aliases. No code change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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
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.
Per the design at DESIGN-oo-rewrite.md.
Spond.get_*now returns typed Pydantic models (Event,Group,Member,Match,Chat, …) with per-instance methods (event.update(...),event.change_response(...),member.send_message(...),chat.send(...)). Existing dict-style consumers keep working through aDeprecationWarningshim. Legacy write methods (update_event/change_response/get_event_attendance_xlsx) emit deprecation warnings and delegate to the new methods.Implementation complete on this branch. 62 tests pass (split by domain across
tests/conftest.py+ 7test_*.pyfiles); live-tested end-to-end against the real Spond API.🤖 Generated with Claude Code