Corrected the labeling notification to proper BIDS

[CodyCBakerPhD]

Noticed while putting together #173

[CodyCBakerPhD]

A follow-up PR will add a common registry for notifications to avoid the need for this ctrl+r duplication (as discussed in #103)

[codecov]

Codecov Report

❌ Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.51%. Comparing base (b40ee40) to head (e3e7bd2).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
src/nwb2bids/_command_line_interface/_main.py	0.00%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #176      +/-   ##
==========================================
- Coverage   87.02%   86.51%   -0.51%     
==========================================
  Files          32       32              
  Lines        1025     1031       +6     
==========================================
  Hits          892      892              
- Misses        133      139       +6     

Flag	Coverage Δ
unittests	86.51% <14.28%> (-0.51%)	⬇️

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% <ø> (ø)
src/nwb2bids/bids_models/_model_globals.py	100.00% <100.00%> (ø)
src/nwb2bids/bids_models/_participant.py	95.34% <ø> (ø)
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]

Pointed out a small possible issue to look into.

[candleindark]

What do we do with the case notification.severity == Severity.CRITICAL

[CodyCBakerPhD]

Those more directly relate to the case where nwb2bids produces an 'invalid' BIDS directory

I think in a follow-up we can maybe warn that this might be the case, though until various BEPs get merged on BIDS side, this warning will nearly always occur so maybe I'd not add that until things stabilize a bit upstream

[candleindark]

Those more directly relate to the case where nwb2bids produces an 'invalid' BIDS directory

If this is the case, I think we are treading a very fine line here. We are binding the validity of the resulting BIDS directory on the severity of a notification while the severity is more intuitively interpreted as the "severity" of the notification being raised. Also, this special binding is not documented in

nwb2bids/src/nwb2bids/_inspection/_inspection_result.py

Lines 27 to 41 in 0f03ccf

	@enum.unique
	class Severity(enum.Enum):
	"""
	Quantifier of relative severity (how important it is to resolve) for inspection results.
	The larger the value, the more critical it is.
	If an issue can be categorized in multiple ways, the most severe category should be chosen.
	"""
	INFO = enum.auto() # Not an indication of problem but information of status or confirmation
	HINT = enum.auto() # Data is valid but could be improved
	WARNING = enum.auto() # Data is not recognized as valid. Changes are needed to ensure validity
	ERROR = enum.auto() # Data is recognized as invalid
	CRITICAL = enum.auto() # A serious invalidity in data

.

I am not proposing to provide the documentation since I believe the severity level should only be used to signify how severe the notification being raised is. I am proposing to include the notification.severity == Severity.CRITICAL condition. I.e. instead of

any(
        notification.severity == Severity.ERROR for notification in notifications
    )

we should have

any(
        notification.severity == Severity.ERROR  or notification.severity == Severity.CRITICAL for notification in notifications
    )

After all, if the resulting BIDS dataset is invalid, I don't think we should print the message "BIDS dataset was successfully created!"

[CodyCBakerPhD]

Sounds good - it is a separate enhancement to the main topic of this PR however (and would benefit from even more targeted improvements + testing on separate PR) opened #193

[candleindark]

Those more directly relate to the case where nwb2bids produces an 'invalid' BIDS directory

If this is the case, I think we are treading a very fine line here. We are binding the validity of the resulting BIDS directory on the severity of a notification while the severity is more intuitively interpreted as the "severity" of the notification being raised. Also, this special binding is not documented in

nwb2bids/src/nwb2bids/_inspection/_inspection_result.py

Lines 27 to 41 in 0f03ccf

	@enum.unique
	class Severity(enum.Enum):
	"""
	Quantifier of relative severity (how important it is to resolve) for inspection results.
	The larger the value, the more critical it is.
	If an issue can be categorized in multiple ways, the most severe category should be chosen.
	"""
	INFO = enum.auto() # Not an indication of problem but information of status or confirmation
	HINT = enum.auto() # Data is valid but could be improved
	WARNING = enum.auto() # Data is not recognized as valid. Changes are needed to ensure validity
	ERROR = enum.auto() # Data is recognized as invalid
	CRITICAL = enum.auto() # A serious invalidity in data

.

I am not proposing to provide the documentation since I believe the severity level should only be used to signify how severe the notification being raised is. I am proposing to include the notification.severity == Severity.CRITICAL condition. I.e. instead of

any(
        notification.severity == Severity.ERROR for notification in notifications
    )

we should have

any(
        notification.severity == Severity.ERROR  or notification.severity == Severity.CRITICAL for notification in notifications
    )

After all, if the resulting BIDS dataset is invalid, I don't think we should print the message "BIDS dataset was successfully created!"

[CodyCBakerPhD]

@candleindark primary comment offloaded to #193 for further improvement and standalone testing

This PR is all about the core logic of the actual BIDS labelling