Create LICENSE#6
Merged
Merged
Conversation
Olen
added a commit
that referenced
this pull request
May 14, 2026
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.
Olen
added a commit
that referenced
this pull request
May 14, 2026
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).
Olen
added a commit
that referenced
this pull request
May 14, 2026
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.
Olen
added a commit
that referenced
this pull request
May 14, 2026
Five actionable findings, one verified-as-non-issue: Transaction parity with the rest of the OO surface (finding #1): - Transaction was the only DictCompatModel still declared `extra="ignore"` after the prior bulk-switch to `extra="allow"`. The docstring claimed unmodelled fields were accessible via `transaction["someField"]` — that was wrong under `extra="ignore"` (Pydantic discards). Switched to `extra="allow"` for parity; the docstring claim is now accurate. Also defaulted paid_at / payment_name / paid_by_name so a single malformed transaction doesn't crash the pagination over the whole club's history. API-drift resilience for required scalar fields (findings #2, #3): - Same regression class addressed for Event in earlier rounds: required `str` / `datetime` fields without defaults crash the entire `get_*()` call if Spond ever drops one. Extended the sentinel-default pattern to: - Person.first_name / last_name (Member + Guardian inherit) — "" - Profile.first_name / last_name — "" - Group.name — "" - Subgroup.name — "" - Role.name — "" - Post.timestamp — None - Transaction.paid_at / payment_name / paid_by_name (above) Only the strictly-load-bearing `uid` stays required across all typed models. Event.update doesn't send `null` for unset optionals (finding #4): - model_dump now uses `exclude_none=True` in addition to the read-only field exclusion. Spond could interpret `"description": null` as "clear this field" rather than "leave unchanged"; the pre-OO _EVENT_TEMPLATE-merge approach never sent None for fields the existing event didn't have. New regression test `test_event_update_excludes_none_fields` locks this in. Person.profile vs Profile asymmetry doc (finding #5): - `get_person()` returns `Person` whose `.profile` is a sparse raw dict, NOT a typed `Profile` (which only `get_profile()` returns). Added an explicit "Note:" paragraph to the `get_person` docstring explaining the difference. Also tightened the field docstring on `Person.profile` to explain *why* it's not a typed Profile (the two shapes diverge in Spond's API). LenientDate "duplication" (finding #6) — verified non-issue: - The `LenientDate` annotated type is defined once in `_compat.py` and imported by both `Profile` and `Member`. What looks like duplication is just two consumers of the same import. No action. Proactive: the same "required field without default" pattern was checked across every typed model and relaxed where it occurred. New test `test_models_survive_missing_optional_fields` covers Member/Post/Profile construction from minimal payloads. 52 tests pass; ruff clean; format clean. Live-tested: real API responses parse cleanly (8 groups, events, posts with populated timestamps).
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.
No description provided.