docs+fix: expand docstrings, sanitise AuthenticationError message#241
Merged
Conversation
Follow-up to #237. The first pass fixed inaccuracies but left many docstrings thin or absent — once they were rendered through pdoc on the GitHub Pages site, the gaps were obvious. This pass aims for every public function/method/class (and every private helper that has non-obvious behaviour) to have a real description that explains *why* and *when*, not just the parameter list. Coverage: - Module-level docstrings: spond.spond, spond.base, spond.club. - Class docstrings: Spond, SpondClub, _SpondBase, with usage examples on the two concrete clients. - __init__ docstrings: Spond, SpondClub, _SpondBase. - Method docstrings: get_profile, get_groups, get_group, get_person, get_messages, send_message, get_events, get_event, update_event, get_event_attendance_xlsx, change_response, get_transactions — each expanded to cover caching behaviour, response shape, edge cases, and (where applicable) cross-references to other methods. - Private helpers: _login_chat, _match_person, _get_entity, _continue_chat, _extract_access_token — documented so future maintainers can understand the call graph. - AuthenticationError: expanded with common causes (wrong creds, 2FA, rate limits, API shape changes) since the bare "auth failed" message isn't actionable. Notes embedded as cross-references where relevant: - `send_message` — points at #238 (missing await + return-type bugs). - `update_event` — points at #239 (returns cached events instead of update response). - `get_event` — points at #137/#138/#236 (cache filter limitation being fixed via direct fetch). - `Spond.__init__` — points at #205 (2FA not supported). No code changes — purely documentation. All 23 tests still pass and pdoc regenerates cleanly.
6 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
Expands and enriches docstrings across the Spond Python SDK’s public API so the generated pdoc site provides practical “why/when/how” guidance, usage examples, and known-caveat context.
Changes:
- Added module-level and class-level docstrings (including usage examples) for the core consumer client, club client, and shared base.
- Expanded docstrings on key public methods and non-obvious helpers to document caching behavior, response shapes, and edge cases.
- Improved authentication-related documentation, including richer
AuthenticationErrorguidance and login response shape notes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
spond/spond.py |
Adds/expands docstrings for the main consumer API client and methods (caching, chats, events, attendance export, helpers). |
spond/club.py |
Adds module/class/init docstrings and expands get_transactions() documentation (pagination/caching). |
spond/base.py |
Adds module/class/init docstrings and documents _extract_access_token() expectations and failure modes. |
spond/__init__.py |
Expands AuthenticationError docstring with common causes and diagnostic hints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Four fixes raised in Copilot's review of #241: - Spond class example: guard `await s.get_groups()` with `or []` so iterating the result is safe when the account has no groups (the return type is documented as `list[JSONDict] | None`). - `get_events()` docstring: removed the forward-looking claim that `get_event(uid)` "queries the singular endpoint directly" — that's only true after #236 lands. Replaced with current behavior plus reference to #236 as the planned change. - `get_events()` `subgroup_id` parameter: docstring said `subgroupId` API parameter, but the code sends `subGroupId` (capital G, line 387). Aligned the docstring with the actual API param. - `update_event()` Returns section: was declared `JSONDict` but the method (due to known bug #239) currently returns `self.events`, a `list[JSONDict] | None`. Updated the Returns type to match the current contract, with the bug and workaround called out in Notes.
Four issues raised by Copilot's review of f132248: 1. Spond.__init__ docstring referenced an undefined `client` variable in the close-session example. Aligned with the `s` convention used in the class-level example. 2. get_event_attendance_xlsx example used `pathlib.Path` without importing pathlib. Added the import so the snippet copy/pastes. 3. AuthenticationError docstring acknowledged that the exception message included the raw login response — but the underlying code actually leaked sensitive fields (2FA challenge tokens, masked phone numbers) into log streams via the message. Fixed properly: - Added a `_SAFE_LOGIN_ERROR_FIELDS` whitelist (error/errorKey/errorCode/message) in spond/base.py. - `_extract_access_token` now builds the AuthenticationError message from only the whitelisted fields, falling back to a placeholder when the response shape is fully unrecognised. - Updated the AuthenticationError docstring to describe the new behaviour and reassure users about logging safety. - Added a regression test (test_extract_access_token__error_message_drops_sensitive_fields) that locks in: 2FA challenge token doesn't appear in the message, `phoneNumber` doesn't appear, but whitelisted `errorKey` does. 4. get_transactions cache caveat — `self.transactions` is per-instance but not keyed by `club_id`, so calling with different club_ids mixes results. Documented this with a "Caching caveat" block plus guidance to reset the cache or use a fresh client per club.
Olen
changed the title
This was referenced May 14, 2026
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.
Summary
Follow-up to #237. The first pass fixed docstring inaccuracies, but once the docs were live on GitHub Pages it was obvious that many functions had either no docstring or just a one-line description. This PR aims for every public function/method/class (and every private helper with non-obvious behaviour) to have a docstring that explains why and when, not just what arguments it takes.
Started as a documentation-only change. During Copilot's review it became clear that one of the docstrings was honestly describing a real security issue (the
AuthenticationErrormessage leaked sensitive fields), so the scope grew to fix that too. See "Code change" section below.Coverage (documentation)
Code change
`spond/base.py` — `_extract_access_token()` previously raised `AuthenticationError(f"Login failed. Response received: {login_result}")`, which on the 2FA-required code path can leak fields like a challenge `token` and `phoneNumber` into application logs. Replaced with a whitelist (`error`, `errorKey`, `errorCode`, `message`) so only safe diagnostic fields surface in the exception message. Other fields are dropped.
`tests/test_spond.py` — new regression test `test_extract_access_token__error_message_drops_sensitive_fields` locks the new behaviour in: a synthetic 2FA-shaped response no longer surfaces the challenge `token` or `phoneNumber` in the message, but the whitelisted `errorKey` still does.
This is a small, focused, no-API-break change. `AuthenticationError` is still raised in the same places with the same type; only the human-readable message content changed.
Cross-references embedded in docstrings
The expanded docstrings link to existing issues/PRs where relevant, so users reading the pdoc site get context on known caveats:
send_message()missingawaiton_continue_chat()and can returnFalsedespiteJSONDictannotation #238 (missing await + return-type bugs)update_event()returns cached events list instead of the API update response #239 (returns cached events list instead of update response)Spond.get_event()may not return an event if more than 100 events are available #137/Spond.get_event()doesn't return 'scheduled' events, i.e. where invites are set to send in the future. #138/fix: fetch single event by uid directly (#137, #138) #236 (cache filter limitation, direct-fetch fix in flight)Test plan
🤖 Generated with Claude Code