Enhance error notifications #193

CodyCBakerPhD · 2025-11-18T23:40:26Z

As requested as further enhancement after #176

codecov · 2025-11-19T19:42:23Z

Codecov Report

❌ Patch coverage is 28.00000% with 36 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.25%. Comparing base (8db95f4) to head (1fec0f7).
⚠️ Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
src/nwb2bids/_command_line_interface/_main.py	0.00%	33 Missing ⚠️
src/nwb2bids/_tools/_pluralize.py	0.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #193      +/-   ##
==========================================
- Coverage   85.79%   84.25%   -1.54%     
==========================================
  Files          33       33              
  Lines        1119     1156      +37     
==========================================
+ Hits          960      974      +14     
- Misses        159      182      +23

Flag	Coverage Δ
unittests	`84.25% <28.00%> (-1.54%)`	⬇️

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

Files with missing lines	Coverage Δ
src/nwb2bids/bids_models/_bids_session_metadata.py	`92.77% <100.00%> (ø)`
src/nwb2bids/bids_models/_probes.py	`100.00% <100.00%> (ø)`
src/nwb2bids/_tools/_pluralize.py	`0.00% <0.00%> (ø)`
src/nwb2bids/_command_line_interface/_main.py	`0.00% <0.00%> (ø)`

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

candleindark · 2025-11-19T20:09:02Z

The tests are failing, so I guess more adjustments are needed?

CodyCBakerPhD · 2025-11-19T20:15:14Z

The tests are failing, so I guess more adjustments are needed?

It is in draft, and missing any form of tests. Will ping when ready for eyes

src/nwb2bids/_command_line_interface/_main.py

CodyCBakerPhD · 2025-11-26T19:11:43Z

@candleindark OK this is ready for eyes now

Had to add new notification and new fixture to showcase the lowest level of printout

The tests showcase the new printout behavior

A follow-up will automatically dump full logs and uncomment those pointers to the file that has full information (I just don't want to overwhelm the CLI window with too much info in any of the cases)

CodyCBakerPhD · 2025-12-04T15:40:22Z

@candleindark Can you take a look at this soon (today or tomorrow)?

candleindark · 2025-12-04T17:03:16Z

@candleindark Can you take a look at this soon (today or tomorrow)?

I will do that as well.

for more information, see https://pre-commit.ci

src/nwb2bids/_command_line_interface/_main.py

candleindark · 2025-12-06T09:22:51Z

src/nwb2bids/_command_line_interface/_main.py

+            )
+        console_notification = rich_click.style(text=text, fg="red")
        rich_click.echo(message=console_notification)
+        return


If you return here, what if the len(criticals) > 0?

Errors take priority over criticals. Both will show up in the full logs (next PR)

candleindark · 2025-12-06T09:33:05Z

src/nwb2bids/_command_line_interface/_main.py

+            "\nBIDS dataset was successfully created, but may not be valid!\n"
+            # TODO: "Please review the full notifications report at {run_config.log_file_path}\n\n"
+        )
+        console_notification = rich_click.style(text=text, fg="yellow")


The color coding seems a bit unconventional here. The why coding a message related to critical issues in yellow while coding a message related to error issues in red in line 145.

'yellow' akin to 'warning'

'red' akin to breaking 'error'

I tend to think critical as more severe than error.

Yes, the naming convention needs a revamp for clarity

candleindark · 2025-12-06T09:57:09Z

src/nwb2bids/_command_line_interface/_main.py

+    if errors:
+        number_of_errors = len(errors)
+
+        top_three = errors[:3]
+        number_to_print = len(top_three)

-    if console_notification != "" and not silent:
+        text = (
+            "\nBIDS dataset was not successfully created!\n"
+            f'{_pluralize(n=number_to_print, phrase="An error was", plural="Some errors were")} '
+            "encountered during conversion.\n"
+        )
+        if number_to_print > 1 and number_of_errors > 3:
+            error_text = "".join(f"\n\t- {error.reason}" for error in top_three)
+            text += (
+                f"The first {number_to_print} of {number_of_errors} are shown below:\n\n"
+                f"{error_text}\n\n"
+                # TODO: "The full log file can be found at {run_config.log_file_path}\n"
+            )
+        console_notification = rich_click.style(text=text, fg="red")
        rich_click.echo(message=console_notification)
+        return

-    not_any_failures = not any(notification.severity == Severity.ERROR for notification in notifications)
-    if not_any_failures and not silent:
-        text = "\nBIDS dataset was successfully created!\n\n"
-        console_notification = rich_click.style(text=text, fg="green")
+    if criticals:
+        text = (
+            "\nBIDS dataset was successfully created, but may not be valid!\n"
+            # TODO: "Please review the full notifications report at {run_config.log_file_path}\n\n"
+        )
+        console_notification = rich_click.style(text=text, fg="yellow")
        rich_click.echo(message=console_notification)
+        return


These two if statements seem to assume that errors and criticals can not be both nonempty. I think that's a false assumption.

If no such assumption is made, the intent of the block of code is to ignore any elements in criticals if errors is nonempty. Such arrangement binds the Severity.ERROR to "BIDS dataset was not successfully created" and Severity.CRITICAL to "BIDS dataset was successfully created, but may not be valid" when there is no issue with the severity of Severity.ERROR. This kind of binding that I suggested to avoid in #176 (comment).

It's an order of precedence

If any errors, those and only those take priority as information back to the user

If not any errors but some critical, those are then displayed in a less scary manner

Overall trying to limit the amount of output the CLI can flood in these events

All notifications will be included in the linked log dump, so if user checks that they will know all

candleindark · 2025-12-06T10:26:39Z

src/nwb2bids/bids_models/_bids_session_metadata.py

+        messages += self._internal_messages.copy()
        if self.events is not None:
-            messages += self.events.messages
+            messages += self.events.messages.copy()
        if self.probe_table is not None:
-            messages += self.probe_table.messages
+            messages += self.probe_table.messages.copy()
        if self.channel_table is not None:
-            messages += self.channel_table.messages
+            messages += self.channel_table.messages.copy()
        if self.electrode_table is not None:
-            messages += self.electrode_table.messages
+            messages += self.electrode_table.messages.copy()


Why do we need any of these .copy method call at all?

To avoid any potential mutation of upstream attributes

I see - many of these (but not all) are computed fields so meaningless to copy (though I guess doesn't harm anything either way)

Will do a holistic review in an upcoming standardization of notification handling

The copy method calls are not needed at all in any field, computed or not since the += does not mutate its right operand as demo in the following script.

l1 = [1, 2, 3] l2 = l1 l3 = [4, 5, 6] l1 += l3 print(f"l1: {l1}") print(f"l2: {l2}") print(f"l3: {l3}") for idx in range(len(l1)): l1[idx] += 10 print("=== After modification ===") print(f"l1: {l1}") print(f"l2: {l2}") # This shows that l1 has been mutated in place, affecting l2 print(f"l3: {l3}") # l3 remains unchanged, showing right operand of `+=` was not mutated

I mean mutation of deeper elements (the inspection results themselves are mutable)

Def not an issue with computed fields since no underlying attribute to mutatate (fresh every time)

Haven't verified that a simple list copy also copies the models it contains however, that was simply an assumption on my part

I mean mutation of deeper elements (the inspection results themselves are mutable)

Def not an issue with computed fields since no underlying attribute to mutatate (fresh every time)

Haven't verified that a simple list copy also copies the models it contains however, that was simply an assumption on my part

If you want copies of the elements with in the lists as well, the copy method calls are not going to do it since the method only does shallow copy. You can look into https://docs.python.org/3/library/copy.html#copy.deepcopy though. Why are you concerned about mutation of InspectionResult objects, aren't they supposed to be "read-only" once they are created? I don't think any code should attempt to modify one once its is created.

I do think the sanitization does mutate the severity of certain things from higher to lower after sanitization is performed

Might be better ways of doing that in conjunction with frozen models tho

src/nwb2bids/bids_models/_probes.py

…cation raising for new notification

CodyCBakerPhD · 2025-12-06T18:23:00Z

tests/integration/test_remote_convert_nwb_dataset.py

+                "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.json",
+                "sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.tsv",


😜 That's fun - while debugging the new test case for this PR also fixed a separate issue about faulty skipping of probe metadata

CodyCBakerPhD added 2 commits November 18, 2025 16:12

enhance per suggestions

1aaad89

enhance per suggestions

0a25661

CodyCBakerPhD requested a review from candleindark November 18, 2025 23:40

CodyCBakerPhD self-assigned this Nov 18, 2025

CodyCBakerPhD mentioned this pull request Nov 18, 2025

Corrected the labeling notification to proper BIDS #176

Merged

CodyCBakerPhD added the enhancement New feature or request label Nov 18, 2025

Base automatically changed from fix_label_notification to main November 19, 2025 19:17

Merge branch 'main' into enhance_error_notifications

d504162

CodyCBakerPhD and others added 5 commits November 20, 2025 14:07

Merge branch 'main' into enhance_error_notifications

637d083

Merge branch 'main' into enhance_error_notifications

8272663

add tests; required new fixture and new notification

f338a98

add tests; required new fixture and new notification

7097c60

add tests; required new fixture and new notification

f1bea9c

CodyCBakerPhD commented Nov 26, 2025

View reviewed changes

src/nwb2bids/_command_line_interface/_main.py Show resolved Hide resolved

CodyCBakerPhD marked this pull request as ready for review November 26, 2025 19:10

Merge branch 'main' into enhance_error_notifications

6487279

CodyCBakerPhD added the minor Increment the minor version when merged label Dec 1, 2025

Merge branch 'main' into enhance_error_notifications

7c4dde6

Merge branch 'main' into enhance_error_notifications

69f0c74

CodyCBakerPhD and others added 3 commits December 5, 2025 12:36

Merge branch 'main' into enhance_error_notifications

2edc49c

Merge branch 'main' into enhance_error_notifications

ee88e92

[pre-commit.ci] auto fixes from pre-commit.com hooks

fb34d6a

for more information, see https://pre-commit.ci

candleindark requested changes Dec 6, 2025

View reviewed changes

Merge branch 'main' into enhance_error_notifications

25a5230

CodyCBakerPhD and others added 4 commits December 6, 2025 12:07

Merge branch 'main' into enhance_error_notifications

82f4876

add more print header conditions

1f46a75

add API test for new info message; fix new pytest fixture; fix notifi…

7c42b6b

…cation raising for new notification

adjust datalad test to new expectation

6d3b066

CodyCBakerPhD commented Dec 6, 2025

View reviewed changes

CodyCBakerPhD and others added 2 commits December 7, 2025 12:53

final review

e95399d

Merge branch 'main' into enhance_error_notifications

1fec0f7

CodyCBakerPhD merged commit f5cc02c into main Dec 7, 2025
27 checks passed

CodyCBakerPhD deleted the enhance_error_notifications branch December 7, 2025 18:07

		"sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.json",
		"sub-fCamk1_ses-fCamk1_200827_sess9_no_raw_data_probes.tsv",

Enhance error notifications #193

Enhance error notifications #193

Uh oh!

Conversation

CodyCBakerPhD commented Nov 18, 2025

Uh oh!

codecov bot commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

candleindark commented Nov 19, 2025

Uh oh!

CodyCBakerPhD commented Nov 19, 2025

Uh oh!

Uh oh!

CodyCBakerPhD commented Nov 26, 2025

Uh oh!

CodyCBakerPhD commented Dec 4, 2025

Uh oh!

candleindark commented Dec 4, 2025

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 19, 2025 •

edited

Loading