Improve and bug fix `_validate_bids_directory` by candleindark · Pull Request #323 · con/nwb2bids

candleindark · 2026-02-04T06:04:35Z

This PR improves _validate_bids_directory, which is used to validate the bids_directory field in the RunConfig model.

Particularly,

It correct the handling of the situation in which the given path points to a file and the situation in which the parent of the given path points to a file. (The implementation before this PR change contains a bug in handling these situations).
It improves the docstring of the function.

The name of `path` is more appropriate since there is no guarantee that it is a path to a directory

Copilot

Pull request overview

This PR improves the _validate_bids_directory function to correctly handle edge cases where paths or their parents point to files rather than directories. The previous implementation had bugs that would either produce unclear errors or incorrectly pass validation in these scenarios.

Changes:

Fixed validation logic to properly distinguish between directories and files for both the target path and its parent
Improved error messages to provide clearer feedback about validation failures
Enhanced the docstring with comprehensive documentation of validation criteria, parameters, return values, and exceptions

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-02-04T06:20:54Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.88%. Comparing base (bcaa55e) to head (398368d).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #323      +/-   ##
==========================================
+ Coverage   84.80%   84.88%   +0.08%     
==========================================
  Files          39       39              
  Lines        1494     1502       +8     
==========================================
+ Hits         1267     1275       +8     
  Misses        227      227

Flag	Coverage Δ
unittests	`84.88% <100.00%> (+0.08%)`	⬆️

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

Files with missing lines	Coverage Δ
src/nwb2bids/_core/_validate_existing_bids.py	`85.71% <100.00%> (+1.84%)`	⬆️

🚀 New features to boost your workflow:

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

So that the case of a given path that points to a file is handled properly

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

CodyCBakerPhD · 2026-02-04T15:24:24Z

+    if not path.parent.is_dir():
+        if not path.parent.exists():
+            raise ValueError(f"The parent path ({path.parent}) does not exist.")


How does this make any sense? At this point you have passed the check that the path exists, so how could it exist without a parent?

At this point, path is actually confirmed to not exist.

The following demo the case that the logic is handling.

from pathlib import Path from nwb2bids._core._validate_existing_bids import _validate_bids_directory p = Path("some_missing_parent/child") try: _validate_bids_directory(p) except ValueError as e: print(e) """ The parent path (some_missing_parent) does not exist. """

CodyCBakerPhD · 2026-02-04T15:25:02Z

+    if path.is_dir():
+        return _validate_existing_directory_as_bids(path)
+    if path.exists():
+        raise ValueError(f"The path ({path}) exists but is not a directory.")


These checks might make more sense re-ordered as follows:

check the parent properties -> check the current path is not a file -> do the full BIDS check

I am really just building on the existing implementation,

nwb2bids/src/nwb2bids/_core/_validate_existing_bids.py

Lines 5 to 13 in 972adeb

def _validate_bids_directory(directory: pathlib.Path) -> pathlib.Path:

"""Validate bids_directory: if exists, must be valid BIDS; if not, parent must exist."""

if directory.exists():

return _validate_existing_directory_as_bids(directory)

if not directory.parent.exists():

raise ValueError(f"parent directory does not exist: {directory.parent}")

return directory

. I just inserted logic to handle two additional situations, the situation in which the given path points to a file and the situation in which the parent of the given path points to a file.

While what you are suggesting here seems reasonable, the implementation proposed by this PR is more or less in the reverse order of what you are suggesting, trigger logic to handle a problem only when it occurs instead of checking preemptively like what you may be suggesting.

The two additional situations mentioned above are demonstrated by the two added tests https://github.com/candleindark/nwb2bids/blob/398368d5c8a9f5f913284a08a4176a5543ba2e2c/tests/unit/test_run_config.py#L37-L59.

CodyCBakerPhD · 2026-02-04T15:30:29Z

+    if not path.parent.is_dir():
+        if not path.parent.exists():


I'm pretty sure that path.parent.is_file() also checks for existence

Yes, it does, but I am not just checking if path.parent is a file. I have raise ValueError(f"The parent path ({path.parent}) exists but is not a directory.") in the outer if-statement as well.

CodyCBakerPhD · 2026-02-04T15:33:27Z

+    - it points to an existing BIDS directory, which contains a valid `dataset_description.json` file.
+    - it points to an empty directory, in which case a minimal `dataset_description.json` file will be
+      created and added to the directory.
+    - it does not point to an object in the file system, but its parent exists as a directory.


Remind me why we care about the parent at all?

This is an adherence to the previous documentation and implementation,

nwb2bids/src/nwb2bids/_core/_validate_existing_bids.py

Line 6 in 972adeb

"""Validate bids_directory: if exists, must be valid BIDS; if not, parent must exist."""

.

rf(rename): rename param in _validate_bids_directory

a1e9818

The name of `path` is more appropriate since there is no guarantee that it is a path to a directory

candleindark added bug Something isn't working internal Changes only affect the internal API labels Feb 4, 2026

candleindark requested a review from Copilot February 4, 2026 06:04

Copilot started reviewing on behalf of candleindark February 4, 2026 06:05 View session

candleindark changed the title ~~Improve _validate_bids_directory~~ Improve and bug fix _validate_bids_directory Feb 4, 2026

Copilot AI reviewed Feb 4, 2026

View reviewed changes

candleindark force-pushed the enh-_validate_bids_directory branch from 6e98902 to 64ef0b8 Compare February 4, 2026 06:36

candleindark added 3 commits February 3, 2026 22:54

fix: correct logic in _validate_bids_directory

6d8a96e

So that the case of a given path that points to a file is handled properly

doc: improve docstring for _validate_bids_directory

d94e65e

test: update test_run_config_missing_parent_raises

2a9bef8

candleindark force-pushed the enh-_validate_bids_directory branch from 8778356 to 6926073 Compare February 4, 2026 06:55

candleindark requested a review from Copilot February 4, 2026 06:56

Copilot started reviewing on behalf of candleindark February 4, 2026 06:59 View session

Copilot AI reviewed Feb 4, 2026

View reviewed changes

Comment thread tests/unit/test_run_config.py Outdated

test: provide additional tests for bids_directory validation

398368d

candleindark force-pushed the enh-_validate_bids_directory branch from 6926073 to 398368d Compare February 4, 2026 07:07

candleindark marked this pull request as ready for review February 4, 2026 07:07

CodyCBakerPhD assigned candleindark Feb 4, 2026

CodyCBakerPhD reviewed Feb 4, 2026

View reviewed changes

	def _validate_bids_directory(directory: pathlib.Path) -> pathlib.Path:
	"""Validate bids_directory: if exists, must be valid BIDS; if not, parent must exist."""
	if directory.exists():
	return _validate_existing_directory_as_bids(directory)

	if not directory.parent.exists():
	raise ValueError(f"parent directory does not exist: {directory.parent}")

	return directory

Conversation

candleindark commented Feb 4, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Feb 4, 2026 •

edited

Loading