Update for type-safe ActivityRecord fields and legacy migration helpers#260
Open
jonathanMLDev wants to merge 4 commits into
Open
Update for type-safe ActivityRecord fields and legacy migration helpers#260jonathanMLDev wants to merge 4 commits into
jonathanMLDev wants to merge 4 commits into
Conversation
📝 WalkthroughWalkthroughThis PR implements type safety for the ChangesActivity Record Type Safety
Sequence Diagram(s)sequenceDiagram
participant LegacyDict as Legacy String Dict
participant MigrateFn as migrate_legacy_activity_fields
participant TypedFields as Typed Components
participant SerializeFn as activity_record_to_legacy_dict
participant NewDict as New Legacy Dict
LegacyDict->>MigrateFn: source_system, occurred_at, activity_type, actor_id
MigrateFn->>TypedFields: SourceSystem, datetime, ActivityType, ActorExternalId
TypedFields->>SerializeFn: normalized typed values
SerializeFn->>NewDict: formatted iso strings, enum values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Collaborator
Author
|
@coderabbitai full review again |
|
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
core/tests/test_protocols.py (1)
108-144: 💤 Low valuePython version inconsistency between negative Pyright tests.
This test uses
pythonVersion: "3.13"(line 118) whiletest_pyright_negative_protocol_assignment_fileusespythonVersion: "3.11"(line 158). Consider aligning both to the same version for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/tests/test_protocols.py` around lines 108 - 144, The test function test_pyright_negative_activity_record_assignment_file sets "pythonVersion": "3.13" in the pyright config JSON, which is inconsistent with test_pyright_negative_protocol_assignment_file that uses "3.11"; update the pythonVersion in the JSON written in test_pyright_negative_activity_record_assignment_file to match the other test (e.g., "3.11") so both negative Pyright tests use the same Python version.core/activity_types.py (1)
93-101: ⚡ Quick winRedundant conversion: avoid converting datetime to string and back.
On line 98, when
rawis already adatetime, the code converts it to a string (str(raw)) only to parse it back. This is inefficient and unnecessary—just callensure_activity_occurred_at(raw)directly on line 97.♻️ Proposed refactor to eliminate redundant conversion
def _parse_occurred_at(raw: str | datetime | None) -> datetime | None: if raw is None: return None if isinstance(raw, datetime): return ensure_activity_occurred_at(raw) - text = str(raw).strip() + text = raw.strip() if not text: return None return parse_activity_occurred_at(text)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@core/activity_types.py` around lines 93 - 101, The _parse_occurred_at function currently converts the input to string and parses it even when raw is already a datetime; instead, detect datetime first and early-return ensure_activity_occurred_at(raw) so you never call str(raw) for datetime values. Update _parse_occurred_at to call ensure_activity_occurred_at(raw) and return immediately when isinstance(raw, datetime), and only run str(raw).strip() + parse_activity_occurred_at(text) for non-datetime inputs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@core/tests/test_activity_types.py`:
- Around line 45-48: The test test_ensure_activity_occurred_at_converts_non_utc
is using a UTC datetime but should verify conversion from a non-UTC zone; change
the input datetime in the test to use a non-UTC tzinfo (for example
timezone(timedelta(hours=-5))) and call ensure_activity_occurred_at, then assert
the returned datetime has tzinfo == timezone.utc and that the instant/time value
was correctly normalized to UTC (i.e., the converted time represents the same
absolute moment as the original non-UTC input).
---
Nitpick comments:
In `@core/activity_types.py`:
- Around line 93-101: The _parse_occurred_at function currently converts the
input to string and parses it even when raw is already a datetime; instead,
detect datetime first and early-return ensure_activity_occurred_at(raw) so you
never call str(raw) for datetime values. Update _parse_occurred_at to call
ensure_activity_occurred_at(raw) and return immediately when isinstance(raw,
datetime), and only run str(raw).strip() + parse_activity_occurred_at(text) for
non-datetime inputs.
In `@core/tests/test_protocols.py`:
- Around line 108-144: The test function
test_pyright_negative_activity_record_assignment_file sets "pythonVersion":
"3.13" in the pyright config JSON, which is inconsistent with
test_pyright_negative_protocol_assignment_file that uses "3.11"; update the
pythonVersion in the JSON written in
test_pyright_negative_activity_record_assignment_file to match the other test
(e.g., "3.11") so both negative Pyright tests use the same Python version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a740a90b-1853-4914-9356-9e40dd7b06db
⛔ Files ignored due to path filters (2)
requirements-dev.lockis excluded by!**/*.lockrequirements.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
CHANGELOG.mdcore/README.mdcore/activity_types.pycore/protocols.pycore/pyright_samples/activity_record_assignment_negative.pycore/tests/test_activity_types.pycore/tests/test_protocols.pydiscord_activity_tracker/protocol_impl.pydiscord_activity_tracker/tests/test_protocol_impl.pydocs/Core_public_API.mddocs/service_api/core_protocols.mdgithub_activity_tracker/protocol_impl.pyrequirements.in
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.
Summary
Completes typed
ActivityRecordsupport for the portable activity DTO used at tracker sync boundaries. Replaces untypedstrfields oncore.protocols.ActivityRecordwith structured types and adds a shared migration layer for legacy string-keyed export/bridge payloads.Addresses the valid scope of the issue: typed
occurred_at,activity_type,source_system, andactor_external_id, plus legacy migration and Pyright boundary checks. Out of scope: Slack/mailingActivityRecordadopters, eval narrative updates.Changes
core/activity_types(new)SourceSystem—StrEnum(github,discord)ActivityType— branded frozen dataclass withgithub_issue()anddiscord_message(subtype)factoriesActorExternalId—NewType+actor_external_id()helperLegacyActivityRecordDict—TypedDictfor export/bridge JSONparse_activity_occurred_at/ensure_activity_occurred_at— aware UTC datetimes viacore.utils.datetime_parsingmigrate_legacy_activity_fields— string/legacy input → typed tupleactivity_record_to_legacy_dict— inverse forto_legacy_dict()on dataclassescore/protocolsActivityRecordprotocol properties now use the types above (occurred_at:datetime | None)Tracker implementations
github_activity_tracker/protocol_impl.py—GitHubActivityRecorduses typed fields;from_issue,from_legacy_dict,to_legacy_dictdiscord_activity_tracker/protocol_impl.py—DiscordActivityRecorduses typed fields;from_converted_export_dict,to_legacy_dictTests and typing
core/tests/test_activity_types.py— parse, migrate, round-trip, unknownsource_systemcore/pyright_samples/activity_record_assignment_negative.py— Pyright-negative sample for invalidActivityRecordassignmentscore/tests/test_protocols.py— existing subprocess Pyright test wired to the new sampleDocs
core/README.md— listsactivity_types.pyCHANGELOG.md— already notes ActivityRecord typing under[Unreleased]Test plan
pytest core/tests/test_activity_types.py core/tests/test_protocols.py discord_activity_tracker/tests/test_protocol_impl.py github_activity_tracker/tests/test_protocol_impl.pycore/activity_types.py,core/protocols.py, bothprotocol_implmodules, andactivity_record_assignment_negative.pyNotes
Zstrings viato_legacy_dict()/migrate_legacy_activity_fields; no ORM migration required (ActivityRecordis not a Django model).ActivityRecordtoday; other collectors unchanged.Related issue
Summary by CodeRabbit
New Features
Refactor
Documentation
Tests
Chores