Skip to content

Add NotificationSummary class for structured, version-stamped inspection reports#353

Open
CodyCBakerPhD with Copilot wants to merge 12 commits into
mainfrom
copilot/add-notification-summary-class
Open

Add NotificationSummary class for structured, version-stamped inspection reports#353
CodyCBakerPhD with Copilot wants to merge 12 commits into
mainfrom
copilot/add-notification-summary-class

Conversation

Copilot AI commented Mar 19, 2026

Copy link
Copy Markdown
Contributor
  • Explore codebase and understand notification system
  • Create NotificationSummary class in src/nwb2bids/notifications/_notification_summary.py
    • Takes list[Notification] and optional run_id as input
    • __str__ override with nice human-readable report (aggregates by identifier)
    • to_json() returns full JSON string (all individual notifications, no aggregation)
    • to_file(path, aggregate=True) writes to file (.json or .txt based on suffix)
  • Export NotificationSummary from notifications/__init__.py and top-level __init__.py
  • Update DatasetConverter.convert_to_bids_dataset() to use NotificationSummary and write both .txt and .json files
  • Add unit tests in tests/unit/test_notification_summary.py (15 tests)
  • Update integration test test_notifications.py to match new JSON file structure and verify .txt report
  • Fix tests/integration/test_convert_nwb_dataset.py::test_minimal_convert_nwb_dataset_from_directory for new JSON format
  • Resolve merge conflicts from origin/main (dataset_converter.py and test_convert_nwb_dataset.py)
  • All tests pass
Original prompt

This section details on the original issue you should resolve

<issue_title>Generate nice 'report' of inspections</issue_title>
<issue_description>Right now I just form and dump a list of messages through .model_dump on each message of a DatasetConverter

Would be nice to create (and version-stamp) official report structure in both human readable text and JSON, similar to BIDs and NWB Inspector</issue_description>

Comments on the Issue (you are @copilot in this section)

@CodyCBakerPhD Plan:
  • introduce a new class, the NotificationSummary which takes a list of notifications as input
  • this class should have a nice-looking __str__ override for printing out in places like notebooks and ipython consoles
  • it should have to_json (dumps all information in full) and to_file (with a flag determining whether to aggregate over similar types of notifications or show all information; default is to aggregate methods)
  • this should be used in place of the other method which dumps notifications to the .nwb2bids directory and should dump a copy of both files by default</comment_new>

📱 Kick off Copilot coding agent tasks wherever you are with GitHub Mobile, available on iOS and Android.

- Add `NotificationSummary` class in `notifications/_notification_summary.py`
  with `__str__`, `to_json()`, and `to_file(path, aggregate=True)` methods
- Export `NotificationSummary` from `notifications/__init__.py` and top-level `__init__.py`
- Update `DatasetConverter.convert_to_bids_dataset()` to use `NotificationSummary`
  and write both `.json` and `.txt` report files
- Update integration test to match new JSON structure (dict with version/run_id/notifications)
- Add unit tests in `tests/unit/test_notification_summary.py`

Co-authored-by: CodyCBakerPhD <51133164+CodyCBakerPhD@users.noreply.github.com>
Copilot AI changed the title [WIP] Add report generation for inspections with NotificationSummary class Add NotificationSummary class for structured, version-stamped inspection reports Mar 19, 2026
Copilot AI requested a review from CodyCBakerPhD March 19, 2026 03:51
@CodyCBakerPhD

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request to match your design

The test was comparing the notifications JSON file contents against a bare
list `[]`, but the format changed to a dict with nwb2bids_version, run_id,
and notifications keys. Updated the assertion to match the new structure.

Agent-Logs-Url: https://github.com/con/nwb2bids/sessions/d600b2c5-5586-4217-9b79-b65e50c3c7e4

Co-authored-by: CodyCBakerPhD <51133164+CodyCBakerPhD@users.noreply.github.com>

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request to match your design

Resolved in afa2aa1. The merge conflict itself was already resolved in a prior merge commit, but the rebase onto main introduced a new test (test_minimal_convert_nwb_dataset_from_directory) that was still asserting the old bare-list JSON format for the notifications file. Updated it to match the new dict structure with nwb2bids_version, run_id, and notifications keys.

@CodyCBakerPhD

Copy link
Copy Markdown
Collaborator

@copilot resolve the merge conflicts in this pull request

…ion-summary-class

# Conflicts:
#	src/nwb2bids/_converters/_dataset_converter.py
#	tests/integration/test_convert_nwb_dataset.py

Co-authored-by: CodyCBakerPhD <51133164+CodyCBakerPhD@users.noreply.github.com>

Copilot AI commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

@copilot resolve the merge conflicts in this pull request

Resolved in bb2e601. Merged the latest origin/main into the branch; conflicts in _dataset_converter.py (kept NotificationSummary usage) and test_convert_nwb_dataset.py (kept the new JSON structure assertions) were resolved in favour of this PR's design.

@codecov

codecov Bot commented Apr 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.98496% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.08%. Comparing base (e9682af) to head (d10a60d).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...rc/nwb2bids/notifications/_notification_summary.py 93.70% 8 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #353      +/-   ##
==========================================
+ Coverage   85.42%   86.08%   +0.65%     
==========================================
  Files          41       42       +1     
  Lines        1818     1947     +129     
==========================================
+ Hits         1553     1676     +123     
- Misses        265      271       +6     
Flag Coverage Δ
unittests 86.08% <93.98%> (+0.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
src/nwb2bids/__init__.py 100.00% <100.00%> (ø)
src/nwb2bids/_converters/_dataset_converter.py 86.87% <100.00%> (+0.05%) ⬆️
src/nwb2bids/notifications/__init__.py 100.00% <100.00%> (ø)
...rc/nwb2bids/notifications/_notification_summary.py 93.70% <93.70%> (ø)

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@CodyCBakerPhD CodyCBakerPhD marked this pull request as ready for review April 21, 2026 15:23
@CodyCBakerPhD CodyCBakerPhD added enhancement New feature or request minor Increment the minor version when merged labels Apr 21, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request minor Increment the minor version when merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Generate nice 'report' of inspections

2 participants