feat(snapshots): add raw event export behind include_raw_events flag#14
Open
sujalgoel wants to merge 5 commits intoAOSSIE-Org:mainfrom
Open
feat(snapshots): add raw event export behind include_raw_events flag#14sujalgoel wants to merge 5 commits intoAOSSIE-Org:mainfrom
sujalgoel wants to merge 5 commits intoAOSSIE-Org:mainfrom
Conversation
- Wrap storage.list_contributions call in getattr/callable check, consistent with how other optional storage methods are accessed in this module (list_pending_issue_requests, list_recent_notifications). - Remove redundant getattr on snapshot_config.include_raw_events; field is now a real Pydantic field with a default, so direct access is safe. - Rename test_raw_events_excluded_by_default -> test_raw_events_excluded_when_false; the old name implied the test exercised the default, but it passed include_raw_events=False explicitly.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds Changes
Sequence Diagram(s)sequenceDiagram
participant Config as SnapshotConfig
participant Engine as _write_snapshots
participant Collector as _collect_snapshot_data
participant Storage as StorageAdapter
participant Repo as GitHub_Repo
Config->>Engine: include_raw_events=true
Engine->>Collector: collect(snapshot_config, period_start, period_end, ...)
alt storage supports list_contributions
Collector->>Storage: list_contributions(since=period_start)
Storage-->>Collector: [ContributionEvent,...]
Collector->>Collector: filter by created_at <= period_end, build events.json
Collector-->>Engine: files dict (includes events.json)
else storage lacks list_contributions
Collector-->>Engine: files dict (no events.json)
end
Engine->>Repo: write files (may include events.json)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ghdcbot/engine/snapshots.py`:
- Line 168: This boolean parameter is acceptable for an internal function;
either silence Ruff for this name or make it keyword-only: update the function
signature that currently declares "include_raw_events: bool = False" to be
preceded by a positional-only separator (add "*, include_raw_events: bool =
False") if you want to enforce keyword use, or add a local noqa comment to the
parameter to suppress FBT001/FBT002 warnings while keeping the default as-is
(refer to the function that contains the "include_raw_events" parameter).
In `@tests/test_snapshots.py`:
- Around line 362-498: Add a test that exercises the case where the storage
object does not implement list_contributions so _collect_snapshot_data
gracefully omits events.json; create a MinimalStorage class without
list_contributions (but with any other stubbed methods used elsewhere), call
_collect_snapshot_data with include_raw_events=True and assert that
"events.json" is not present in the returned snapshots, referencing the helper
under test _collect_snapshot_data and the SnapshotConfig/include_raw_events
behavior to ensure the guard using getattr/callable is covered.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e34cc510-d205-4f22-a570-b04c6e0ee864
📒 Files selected for processing (3)
src/ghdcbot/config/models.pysrc/ghdcbot/engine/snapshots.pytests/test_snapshots.py
- Make include_raw_events keyword-only in _collect_snapshot_data to silence Ruff FBT001/FBT002 (boolean positional arg warnings) - Replace timezone.utc with datetime.UTC alias in new tests (UP017) - Add test_raw_events_graceful_when_storage_missing_method: verifies that events.json is omitted silently when storage does not implement list_contributions, documenting the getattr/callable guard behaviour
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/ghdcbot/engine/snapshots.py`:
- Around line 329-338: The events payload currently collects contributions using
only period_start (for event in list_contributions(period_start)) which can
include items after period_end; update the collection to enforce the upper bound
by either calling list_contributions with both period_start and period_end
(e.g., list_contributions(period_start, period_end)) or post-filtering
events_data to only include events whose timestamp/created_at <= period_end
before assigning files["events.json"] (referencing list_contributions,
events_data, period_start, period_end, and files["events.json"]).
In `@tests/test_snapshots.py`:
- Line 504: The parameter `limit` in the `list_recent_notifications` method of
the `MinimalStorage` stub is intentionally unused and should be renamed to
`_limit` to silence ARG002; update the method signature `def
list_recent_notifications(self, limit: int = 1000) -> list[dict]:` to use
`_limit` instead (keep the same type hint and default) and leave the
implementation/return behavior unchanged so linting passes without altering
functionality.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 45e2b41a-3eca-43d2-8768-c7178d8efd08
📒 Files selected for processing (2)
src/ghdcbot/engine/snapshots.pytests/test_snapshots.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_snapshots.py`:
- Line 3: The file mixes UTC and timezone.utc; standardize on
datetime.timezone.UTC by replacing uses of timezone.utc with UTC and update the
import to from datetime import UTC, datetime so all tests in
tests/test_snapshots.py use UTC consistently; search for any datetime(...,
tzinfo=timezone.utc) or references to timezone.utc and change them to tzinfo=UTC
(or pass UTC directly) and remove the unused timezone import.
- Around line 433-468: Add a new unit test function (e.g.,
test_raw_events_respects_period_end) that mirrors
test_raw_events_respects_period_start but verifies the upper-bound filtering:
create a MockStorage with two ContributionEvent objects (one with created_at
inside the period and one after period_end), call _collect_snapshot_data with
include_raw_events=True and the desired period_start/period_end, then assert
snapshots["events.json"]["data"] only contains the in-period event (check length
== 1 and github_user matches the expected in-period contributor); reference
MockStorage, ContributionEvent, _collect_snapshot_data, and the events.json data
structure to locate where to add the test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7d8037ec-2dba-459c-a141-f2d04ef19fd9
📒 Files selected for processing (2)
src/ghdcbot/engine/snapshots.pytests/test_snapshots.py
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Author
|
@shubham5080 would love a review on this when you get a chance! |
Member
|
i will check ! |
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
2 participants
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.
What this does
Adds a
include_raw_eventsflag toSnapshotConfigthat, when enabled, writes aevents.jsonfile alongside the other snapshot files in each snapshot directory.The flag defaults to
False, so existing deployments are unaffected.Changes
config/models.pyinclude_raw_events: bool = FalsetoSnapshotConfigengine/snapshots.pyinclude_raw_eventsfrom config down to_collect_snapshot_data_collect_snapshot_data, usesgetattr(storage, "list_contributions", None)+callable()guard, consistent with how other optional storage methods are called in this moduleevents.jsonwith the same envelope schema as other snapshot files (schema_version,generated_at,org,run_id,period_start,period_end,data)tests/test_snapshots.pylist_contributionstoMockStorageand acontributionslist for test datatest_raw_events_excluded_when_false- flag off, noevents.jsontest_raw_events_included_when_enabled- flag on,events.jsonpresent with correct fieldstest_raw_events_respects_period_start- only events at or afterperiod_startappeartest_write_snapshots_raw_events_via_config- end-to-end: config flag triggers file write viagithub_writerWhy the getattr guard
The call to
storage.list_contributionsis guarded the same waylist_pending_issue_requestsandlist_recent_notificationsare guarded elsewhere in this file. The snapshot module acceptsstorage: Anyand is designed to degrade gracefully if a storage adapter does not implement a given method.Testing
Summary by CodeRabbit
New Features
Tests