From 6955a547ed13c4108b660fd12bdb27ec47908ce3 Mon Sep 17 00:00:00 2001 From: karellopez Date: Mon, 3 Aug 2026 15:49:34 +0200 Subject: [PATCH 1/4] feat(issues): add a findings model and schema-driven filename validation Validation currently answers a yes/no question: BIDSValidator.is_bids returns a boolean and the CLI prints one line per bad file. That output carries no severity, no stable code, and no structure a program can consume. These two modules replace it with typed findings, and give the future content checks somewhere to report to. Engine - issues.py holds the finding model: Severity, Issue, and a DatasetIssues collection. Pure attrs data with no I/O, matching the convention already used by context.py and types/files.py. The field set mirrors the reference validator so output stays interchangeable, and Issue.rule records which schema rule fired. - filename_checks.py holds the check logic, kept separate from the container so each has one job. It reads rules.files from the schema, identifies the rule or rules a path matches, then reports one specific code per kind of failure rather than a single blanket "bad name": NOT_INCLUDED, MISSING_REQUIRED_ENTITY, ENTITY_NOT_IN_RULE, ENTITY_WITH_NO_LABEL, INVALID_ENTITY_LABEL, EXTENSION_MISMATCH, DATATYPE_MISMATCH, INVALID_LOCATION, FILENAME_MISMATCH and ALL_FILENAME_RULES_HAVE_ISSUES. - Scope is names and paths only. Nothing opens a file or reads its contents. - The walk applies the reference validator's default ignores (.git**, .*, sourcedata/, code/, stimuli/, log/) in addition to .bidsignore. Without them dotfiles such as .DS_Store are reported, which the reference never does. - Directory recordings such as CTF .ds are treated as single units: the walk does not descend into them and does not name-check their contents. - Schema lookups are memoised per schema object, so the rule tree is flattened once rather than once per file, and the walk is a generator so a large dataset holds one context at a time. One deliberate difference from the reference validator - A data file outside any recognised datatype directory is reported as INVALID_LOCATION. The reference misses this case, because its suffix matching ignores the datatype and its DATATYPE_MISMATCH check is gated on the parent directory being a known datatype. The legacy is_bids regexes do catch it, since they cover the whole path, so dropping it would lose coverage users already have. Metadata files are exempt, because the inheritance principle lets a .json or .tsv sit higher in the tree than the data it describes. Tests - test_issues.py covers the model: defaults, severities, the collection helpers and a serialisation round trip. - test_filename_checks.py is table driven with one case per issue code, plus the default ignores, .bidsignore, the rule field, catalog completeness, and the datatype-directory case cross-checked against is_bids. - 28 tests pass. ruff, ruff format and mypy strict are clean. - Verified on real MRI, EEG, MEG and PET datasets: the filename findings match those of a reference-parity engine exactly. Docs - docs/filename_issues_module.md explains what the modules add, the architecture with flowcharts, the ten codes, how to run it, how content validation extends the same layer, and a technical reference covering every type and function with the reasoning behind each decision. - docs/example_filename_issues.py runs two ways: with no argument it generates a dataset containing one deliberately broken file per issue code, and with a path it validates your own dataset. --- docs/example_filename_issues.py | 103 +++++ docs/filename_issues_module.md | 461 ++++++++++++++++++++ src/bids_validator/filename_checks.py | 606 ++++++++++++++++++++++++++ src/bids_validator/issues.py | 107 +++++ tests/test_filename_checks.py | 147 +++++++ tests/test_issues.py | 61 +++ 6 files changed, 1485 insertions(+) create mode 100644 docs/example_filename_issues.py create mode 100644 docs/filename_issues_module.md create mode 100644 src/bids_validator/filename_checks.py create mode 100644 src/bids_validator/issues.py create mode 100644 tests/test_filename_checks.py create mode 100644 tests/test_issues.py diff --git a/docs/example_filename_issues.py b/docs/example_filename_issues.py new file mode 100644 index 0000000..a65c6b1 --- /dev/null +++ b/docs/example_filename_issues.py @@ -0,0 +1,103 @@ +"""Runnable example for the filename validation module. + +Two modes: + +* No argument: build a small dataset in a temporary directory that contains one + deliberately broken file per issue code, validate it, and print the findings. + Every code the module can emit is demonstrated. +* With a path: validate your own dataset. + +Usage +----- + python docs/example_filename_issues.py # generated demo + python docs/example_filename_issues.py /path/to/dataset # your own data +""" + +from __future__ import annotations + +import json +import sys +import tempfile +from pathlib import Path + +from bidsschematools.schema import load_schema + +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +# Correctly named files. None of these produce a finding. +VALID_FILES = ( + 'README', + 'sub-01/anat/sub-01_T1w.nii.gz', + 'sub-01/func/sub-01_task-rest_bold.nii.gz', +) + +# One broken file per issue code, with the code each one is expected to raise. +BROKEN_FILES = { + 'sub-01/notes.txt': 'NOT_INCLUDED', + 'sub-01/anat/sub-01_T1w.txt': 'EXTENSION_MISMATCH', + 'sub-01/func/sub-01_bold.nii.gz': 'MISSING_REQUIRED_ENTITY', + 'sub-01/anat/sub-01_acq-_T1w.nii.gz': 'ENTITY_WITH_NO_LABEL', + 'sub-01/anat/sub-01_acq-a!b_T1w.nii.gz': 'INVALID_ENTITY_LABEL', + 'sub-01/anat/sub-01_dir-AP_T1w.nii.gz': 'ENTITY_NOT_IN_RULE', + 'sub-01/anat/acq-x_sub-01_T1w.nii.gz': 'FILENAME_MISMATCH', + 'sub-01/func/sub-01_T1w.nii.gz': 'DATATYPE_MISMATCH', + 'sub-02/anat/sub-01_T1w.nii.gz': 'INVALID_LOCATION', + 'sub-01/sub-01_channels.tsv': 'ALL_FILENAME_RULES_HAVE_ISSUES', +} + + +def build_dataset(root: Path) -> Path: + """Create the example dataset under ``root``.""" + (root / 'dataset_description.json').write_text( + json.dumps({'Name': 'filename example', 'BIDSVersion': '1.11.1'}) + ) + for relpath in (*VALID_FILES, *BROKEN_FILES): + path = root / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'') + return root + + +def validate(root: Path) -> None: + """Validate a dataset and print every filename finding.""" + tree = FileTree.read_from_filesystem(str(root)) + issues = collect_filename_issues(tree, load_schema()) + + errors = issues.by_severity(Severity.ERROR) + warnings = issues.by_severity(Severity.WARNING) + + print(f'dataset: {root}') + print(f'{len(issues)} finding(s): {len(errors)} error(s), {len(warnings)} warning(s)\n') + for issue in issues: + print(f'[{issue.severity.value}] {issue.code}') + print(f' file : {issue.location}') + print(f' detail : {issue.message}') + if issue.rule: + print(f' rule : {issue.rule}') + print(f'\nvalid: {"no, errors found" if issues.has_errors else "yes"}') + + +def run_demo() -> None: + """Build the generated example dataset and validate it.""" + with tempfile.TemporaryDirectory() as tmp: + root = build_dataset(Path(tmp)) + print('Generated example: one broken file per issue code.\n') + for relpath, code in BROKEN_FILES.items(): + print(f' {code:32} {relpath}') + print() + validate(root) + + +def main(argv: list[str]) -> int: + """Run the demo, or validate the dataset given as the first argument.""" + if argv: + validate(Path(argv[0])) + else: + run_demo() + return 0 + + +if __name__ == '__main__': + raise SystemExit(main(sys.argv[1:])) diff --git a/docs/filename_issues_module.md b/docs/filename_issues_module.md new file mode 100644 index 0000000..d7217cd --- /dev/null +++ b/docs/filename_issues_module.md @@ -0,0 +1,461 @@ +# The filename issues module + +Two new modules turn filename checking into structured, machine-readable findings: + +| Module | Role | +|---|---| +| `bids_validator.issues` | The **container**: what a finding is. Pure data, no I/O. | +| `bids_validator.filename_checks` | The **logic**: schema-driven filename and path checks. | + +Scope: names and paths only. Nothing here opens a file or reads its contents. + +## What this adds + +Before, the filename check answered a yes/no question. `BIDSValidator.is_bids(path)` +returned a boolean, and the command line printed a line per bad file: + +``` +/sub-01/anat/oops.nii.gz is not a valid bids filename +``` + +That output cannot tell you *why* the name is wrong, cannot be counted or filtered, +and cannot be consumed by another program. + +Now each problem is a typed `Issue` with a specific code: + +``` +[error] DATATYPE_MISMATCH + file : sub-01/func/sub-01_T1w.nii.gz + detail : the file is in 'func' but its suffix belongs in: anat + rule : rules.files.raw.anat.nonparametric +``` + +The finding says which rule was applied and what exactly failed, and it serialises +straight to JSON. + +## Architecture + +The BIDS schema describes every legal filename: which suffix belongs in which +datatype folder, which entities are required or allowed, which extensions are +permitted. The module reads those rules rather than hardcoding BIDS knowledge. + +```mermaid +flowchart TD + S["BIDS schema: rules.files, rules.entities, objects"] + F["one file path from the dataset tree, name and location only"] + M["find the matching rules"] + C["check the file against them"] + I["Issue: code, severity, location, message, rule"] + D["DatasetIssues"] + S --> M + F --> M + M --> C + C --> I + I --> D +``` + +For one file the flow is: + +```mermaid +flowchart TD + A["one file path"] --> B{"ignored by .bidsignore or a default ignore"} + B -->|"yes"| SKIP["skip it, no finding"] + B -->|"no"| C{"matches any rules.files rule"} + C -->|"no"| NI["NOT_INCLUDED"] + C -->|"yes"| N["narrow to the best candidate rule"] + N --> CH["check entities, datatype, extension, location, order"] + CH --> OK["all good: no finding"] + CH --> ISS["one specific code per failure"] +``` + +Default ignores mirror the reference TypeScript validator: `.git**`, `.*`, +`sourcedata/`, `code/`, `stimuli/`, `log/`. Directory recordings such as CTF `.ds` +are treated as single units and are not name-checked inside. + +### Where the codes come from + +Schema-defined `rules.checks` carry their own issue code, but the structural +filename failures do not exist in the schema. Their codes come from the reference +TypeScript validator's catalog (`src/issues/list.ts`). `filename_checks.FILENAME_ISSUES` +mirrors that catalog so the provenance is explicit and the output stays +interchangeable with the reference. + +## The issue codes + +All ten are errors. The reference defines no filename warnings. + +| Code | Raised when | +|---|---| +| `NOT_INCLUDED` | the name matches no BIDS rule at all | +| `MISSING_REQUIRED_ENTITY` | a required entity for that suffix is absent | +| `ENTITY_NOT_IN_RULE` | an entity is not allowed for that suffix | +| `ENTITY_WITH_NO_LABEL` | an entity has no label, such as `acq-` | +| `INVALID_ENTITY_LABEL` | a label breaks the schema's format pattern | +| `EXTENSION_MISMATCH` | the extension is not allowed for that suffix | +| `DATATYPE_MISMATCH` | the datatype folder does not match the suffix | +| `INVALID_LOCATION` | a valid name in the wrong directory, or a data file outside any datatype directory | +| `FILENAME_MISMATCH` | entities duplicated or out of canonical order | +| `ALL_FILENAME_RULES_HAVE_ISSUES` | several rules matched and each had a problem | + +## One deliberate difference from the reference validator + +The module is stricter in exactly one place: **a data file that is not inside a +recognised datatype directory**. + +``` +sub-01/foo/sub-01_T1w.nii.gz a folder that is not a datatype +sub-01/sub-01_T1w.nii.gz no datatype folder at all +``` + +The reference TypeScript validator does not report these. Its suffix matching +ignores the datatype, so the T1w rule still matches, and its `DATATYPE_MISMATCH` +check is then skipped because the parent directory is not a known datatype +(`findDatatype` returns an empty string, and the check is gated on that value being +truthy). + +The legacy `BIDSValidator.is_bids` does report them, because its regexes cover the +whole path including the datatype directory. Since this module replaces that check, +dropping the case would lose coverage users already have, so it is reported as +`INVALID_LOCATION`, whose catalog reason is exactly "The file has a valid name, but +is located in an invalid directory." + +Metadata files are exempt. The inheritance principle lets a `.json` or `.tsv` sit +higher in the tree than the data it describes, so `sub-01/sub-01_T1w.json` and +`task-rest_bold.json` at the dataset root are correct and are not flagged. The +exempt extensions are in `INHERITABLE_EXTENSIONS`. + +Everything else matches the reference exactly. + +## How to use it + +### On your own dataset + +```python +from bidsschematools.schema import load_schema + +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.types.files import FileTree + +tree = FileTree.read_from_filesystem('/path/to/dataset') +issues = collect_filename_issues(tree, load_schema()) + +print(len(issues), 'finding(s)') +for issue in issues: + print(issue.code, issue.location, issue.message) + +if issues.has_errors: + raise SystemExit(1) +``` + +`DatasetIssues` supports `len()`, iteration, `has_errors`, and +`by_severity(Severity.ERROR)`. Each `Issue` converts to a plain dict with +`attrs.asdict(issue)`, so a JSON report is one line: + +```python +import json + +import attrs + +print(json.dumps([attrs.asdict(issue) for issue in issues], indent=2)) +``` + +The example script accepts a dataset path, so you can run it directly: + +```shell +python docs/example_filename_issues.py /path/to/dataset +``` + +### The generated example + +Run it with no argument to build a small dataset containing one deliberately +broken file per issue code, then validate it: + +```shell +python docs/example_filename_issues.py +``` + +The dataset it generates: + +| File | Raises | +|---|---| +| `sub-01/notes.txt` | `NOT_INCLUDED` | +| `sub-01/anat/sub-01_T1w.txt` | `EXTENSION_MISMATCH` | +| `sub-01/func/sub-01_bold.nii.gz` | `MISSING_REQUIRED_ENTITY` | +| `sub-01/anat/sub-01_acq-_T1w.nii.gz` | `ENTITY_WITH_NO_LABEL` | +| `sub-01/anat/sub-01_acq-a!b_T1w.nii.gz` | `INVALID_ENTITY_LABEL` | +| `sub-01/anat/sub-01_dir-AP_T1w.nii.gz` | `ENTITY_NOT_IN_RULE` | +| `sub-01/anat/acq-x_sub-01_T1w.nii.gz` | `FILENAME_MISMATCH` | +| `sub-01/func/sub-01_T1w.nii.gz` | `DATATYPE_MISMATCH` | +| `sub-02/anat/sub-01_T1w.nii.gz` | `INVALID_LOCATION` | +| `sub-01/sub-01_channels.tsv` | `ALL_FILENAME_RULES_HAVE_ISSUES` | + +It also contains correctly named files (`sub-01/anat/sub-01_T1w.nii.gz`, +`sub-01/func/sub-01_task-rest_bold.nii.gz`, `README`) which produce no findings. + +Part of the real output: + +``` +10 finding(s): 10 error(s), 0 warning(s) + +[error] EXTENSION_MISMATCH + file : sub-01/anat/sub-01_T1w.txt + detail : extension '.txt' is not allowed here; allowed: .nii.gz, .nii, .json + rule : rules.files.raw.anat.nonparametric +[error] MISSING_REQUIRED_ENTITY + file : sub-01/func/sub-01_bold.nii.gz + detail : missing required entities: task + rule : rules.files.raw.func.func +[error] FILENAME_MISMATCH + file : sub-01/anat/acq-x_sub-01_T1w.nii.gz + detail : expected filename: sub-01_acq-x_T1w.nii.gz +``` + +## Extending this to file contents + +The issues layer is deliberately generic. An `Issue` does not know what kind of +check produced it, so content validation plugs in without changing anything here. +A content check reads a file and emits the same `Issue` type into the same +`DatasetIssues`: + +```python +def sidecar_checks(context) -> list[Issue]: + """Check the fields inside a JSON sidecar.""" + issues = [] + if 'RepetitionTime' not in context.sidecar: + issues.append( + Issue( + code='SIDECAR_KEY_REQUIRED', + location=context.file.relative_path, + message='RepetitionTime is required for this file', + ) + ) + return issues +``` + +Different codes, one shape: + +```mermaid +flowchart LR + FN["filename checks, today"] --> D["DatasetIssues"] + SC["sidecar field checks"] --> D + NH["NIfTI header checks"] --> D + TC["TSV column checks"] --> D + SR["schema rules.checks"] --> D + D --> T["text report"] + D --> J["JSON"] + D --> S["SARIF"] +``` + +Three things make the extension straightforward: + +1. **The container does not change.** `Issue` and `DatasetIssues` already carry + everything a content finding needs, including `rule` for schema-driven checks. +2. **The context is the natural input.** `Context` already exposes the lazily + loaded contents (`json`, `columns`, `nifti_header`, `sidecar`), so a content + check reads from the same object the filename checks use. +3. **Codes for content checks mostly come from the schema.** Schema-defined + `rules.checks` carry their own `issue` block with a code, level, and message, so + a rule engine can build an `Issue` straight from the schema rather than + hardcoding a catalog. + +The pattern to follow is the one in `filename_checks.py`: a function that takes a +`Context`, returns a `list[Issue]`, and never raises for a file it cannot judge. +Skipping an undeterminable check keeps the validator free of false alarms. + +--- + +# Technical reference + +Everything a developer needs to work on these two modules: the types, the call +graph, and the reason behind each decision. + +## Files + +| File | Role | +|---|---| +| `src/bids_validator/issues.py` | The finding model. Pure data, no imports from the rest of the package. | +| `src/bids_validator/filename_checks.py` | The check logic. Reads the schema, walks the tree, emits findings. | +| `tests/test_issues.py` | Model unit tests. | +| `tests/test_filename_checks.py` | One test per issue code, plus ignore and catalog tests. | +| `docs/example_filename_issues.py` | Runnable example, both modes. | + +## `bids_validator.issues` + +### `Severity(str, Enum)` + +```python +class Severity(str, Enum): + WARNING = 'warning' + ERROR = 'error' +``` + +Subclassing `str` as well as `Enum` means a member *is* a string, so +`attrs.asdict` and `json.dumps` produce `"error"` with no custom encoder and no +`.value` calls at the serialisation boundary. Only two members exist because the +reference validator defines no third level for filename findings; adding one later +does not change any call site. + +### `Issue` + +```python +@attrs.define(kw_only=True) +class Issue: + code: str + severity: Severity = Severity.ERROR + location: str | None = None + message: str | None = None + sub_code: str | None = None + rule: str | None = None +``` + +| Field | Purpose | +|---|---| +| `code` | Stable identifier, the thing tools key on. The only required field. | +| `severity` | Defaults to `ERROR`, which is correct for every filename finding. | +| `location` | Dataset-relative path, taken from `FileTree.relative_path`. | +| `message` | Human-readable detail, the reference validator's `issueMessage`. | +| `sub_code` | Finer category within a code, for example which entity was at fault. | +| `rule` | Dotted schema path of the rule that fired, for example `rules.files.raw.anat.nonparametric`. | + +`attrs` rather than `pydantic` or `msgspec` because `attrs` is what the rest of the +package already uses (`context.py`, `types/files.py`, `bidsignore.py`); a second +data library would be a new dependency and an inconsistency. `kw_only=True` forces +call sites to name their fields, so an `Issue(...)` literal reads as documentation +and adding a field can never silently shift a positional argument. + +`rule` is populated only by the checks that are scoped to one matched rule +(`MISSING_REQUIRED_ENTITY`, `ENTITY_NOT_IN_RULE`, `DATATYPE_MISMATCH`, +`EXTENSION_MISMATCH`). Whole-file findings such as `NOT_INCLUDED` leave it `None`, +because no single rule produced them. + +### `DatasetIssues` + +```python +@attrs.define +class DatasetIssues: + issues: list[Issue] = attrs.field(factory=list) +``` + +| Member | Purpose | +|---|---| +| `add(issue)` | Append one finding. | +| `extend(issues)` | Append many, used by the per-file loop. | +| `by_severity(severity)` | Filter, preserving insertion order. | +| `has_errors` | Property. Drives the process exit code. | +| `__len__`, `__iter__` | Makes it behave like a collection at call sites. | + +A wrapper rather than a bare `list[Issue]` so that later additions (grouping, +severity rollup, a summary view) do not force every caller to change. `factory=list` +gives each instance its own list; a bare `= []` default would be shared across all +instances. + +## `bids_validator.filename_checks` + +### Public API + +| Name | Signature | Notes | +|---|---|---| +| `collect_filename_issues` | `(tree: FileTree, schema: Namespace) -> DatasetIssues` | The front door. Builds the `Dataset`, walks, collects. | +| `iter_contexts` | `(dataset: Dataset, ignore: HasMatch \| None = None) -> Iterator[Context]` | Yields one `Context` per validatable file. A generator, so memory stays flat on large datasets. | +| `build_ignore` | `(tree: FileTree) -> IgnoreMany` | The defaults plus the dataset's `.bidsignore`. | +| `filename_issues` | `(context: Context) -> list[Issue]` | All findings for one file. The unit a future rule engine would call. | +| `DEFAULT_IGNORES` | `tuple[str, ...]` | Mirrors the reference validator's `defaultIgnores`. | +| `FILENAME_ISSUES` | `dict[str, str]` | The ten codes with the reference's reason text. | + +`filename_issues` takes a `Context` and returns a list rather than mutating a +collection. That keeps it pure and independently testable, and it is the same shape +a content check will have, so the two compose without adapters. + +### Internals: rule identification + +| Function | What it does and why | +|---|---| +| `_file_rules(schema)` | Flattens `rules.files` into `[(dotted_path, leaf_rule)]`. The schema nests rules several levels deep; flattening once makes matching a simple loop and gives every finding a printable rule path. | +| `_collect(node, path, out)` | The recursive walk behind it. A node is a leaf when it has `path`, `stem`, or `suffixes`. | +| `_find_rule_matches(schema, context)` | Every rule the file matches. Skips `rules.files.deriv*` unless `DatasetType` is `derivative`, otherwise derivative-only patterns would validate raw files. | +| `_rule_matches(node, context)` | Three ways a rule can match: an exact `path`, a `stem` glob, or membership in `suffixes`. | +| `_match_stem(node, context)` | `fnmatch.fnmatchcase` for the glob, plus a datatype constraint when the rule has one. Case-sensitive because BIDS names are. | +| `_narrow(schema, context, matched)` | Several rules can match one name. Prefer those sharing the file's datatype, then those whose entities and extension fit. Without this a file would be judged against an unrelated rule and produce misleading codes. | +| `_entities_extensions_fit(...)` | The second narrowing test: the extension is allowed and the file's entities are a subset of the rule's. | + +### Internals: per-file checks + +Each returns `list[Issue]`, so `filename_issues` is a concatenation. + +| Function | Emits | +|---|---| +| `_missing_label(context, matched)` | `ENTITY_WITH_NO_LABEL` for entities whose label is `''`. | +| `_entity_label_check(schema, context)` | `INVALID_ENTITY_LABEL`, using the entity's `format` and that format's `pattern` from `objects.formats`, matched with `re.fullmatch`. | +| `_check_rules(schema, context, matched)` | Dispatches to `_rule_issues`. With several candidates still matching, if any is clean the file is accepted; only if all fail does it emit `ALL_FILENAME_RULES_HAVE_ISSUES`. | +| `_rule_issues(schema, context, matched)` | Runs the four rule-scoped checks below for one candidate rule. | +| `_entity_rule_issues(...)` | `MISSING_REQUIRED_ENTITY` and `ENTITY_NOT_IN_RULE`. | +| `_datatype_mismatch(...)` | `DATATYPE_MISMATCH`. | +| `_extension_mismatch(...)` | `EXTENSION_MISMATCH`. | +| `_invalid_location(context)` | `INVALID_LOCATION`, for both the `sub`/`ses` and the `tpl`/`cohort` hierarchies. | +| `_missing_datatype_directory(context, matched)` | `INVALID_LOCATION` for a data file outside any datatype directory. Fires only when the file has no recognised datatype, its extension is not in `INHERITABLE_EXTENSIONS`, and *every* matched rule declares `datatypes`. The last condition is what keeps `participants.tsv` and friends quiet. This is the one check that is stricter than the reference. | +| `_allowed_datatypes(matched)` | Builds the "expected one of: anat" part of that message. | +| `_reconstruction_failure(schema, context)` | `FILENAME_MISMATCH`. Rebuilds the canonical name from the entities in schema order and compares. This is what catches duplication and reordering. | + +### Internals: schema helpers + +| Function | Why it exists | +|---|---| +| `_entities(context)` | Drops `None`-valued entries. `FileParts` records a filename token with no hyphen (the `dataset` in `dataset_description.json`) as an entity with value `None`; treating those as entities would produce a false `FILENAME_MISMATCH` on every such file. An empty string is kept, because that is its own finding. | +| `_entity_by_short(schema)` | Maps short entity names (`acq`) to their schema definitions. Filenames use short names, the schema keys on long ones. | +| `_ordered_short(schema)` | Entity short names in the schema's canonical filename order, from `rules.entities`. Drives the `FILENAME_MISMATCH` reconstruction. | +| `_short(schema, long_name)` | Long name to short name for one entity. | +| `_directory_recordings(schema)` | Extensions whose schema value ends in `/` (CTF `.ds`, MEF `.mefd`, OME-Zarr). | +| `_dataset_type(context)` | `DatasetType` from `dataset_description.json`, defaulting to `raw`. Catches `KeyError`, `OSError`, and `ValueError` so a missing or malformed description degrades instead of aborting the run. | +| `_is_mapping(node)` | `Namespace` is dict-like but not always a `Mapping` instance, so this accepts either. | + +### Types borrowed from the package + +| Type | From | Used for | +|---|---|---| +| `FileTree` | `types.files` | The indexed dataset. `relative_path`, `name`, `is_dir`, `children`. | +| `Context` | `context` | Per-file facts: `path`, `entities`, `datatype`, `suffix`, `extension`, `file`, `dataset`, `schema`. | +| `Dataset` | `context` | Holds the tree, the schema, and the cached `dataset_description`. | +| `Ignore`, `IgnoreMany`, `HasMatch` | `bidsignore` | Gitignore-style matching. `HasMatch` is a `Protocol`, so `iter_contexts` accepts any matcher. | +| `Namespace` | `bidsschematools` | The schema, with attribute and item access. | + +The module reuses `Context` rather than defining its own file model. It is already +the package's per-file abstraction, it already parses names through `FileParts`, and +a parallel model would drift. + +## Design decisions + +1. **Two modules, container and logic.** `issues.py` imports nothing from the + package, so it can never participate in an import cycle and any future check + module can depend on it. +2. **Schema-driven, nothing hardcoded about BIDS.** Entity names, orders, formats, + suffixes, extensions, and datatypes all come from the schema, so a newer schema + changes behaviour with no code change. Only the ten issue *codes* are constants, + because the schema does not define them. +3. **Memoisation keyed on `id(schema)`.** Four caches (`_RULES_MEMO`, + `_ENTITY_BY_SHORT_MEMO`, `_ORDERED_SHORT_MEMO`, `_DIR_RECORDING_MEMO`) avoid + re-flattening the rule tree for every file. `bidsschematools` caches the schema + object for the process, so its identity is stable. +4. **Skip rather than guess.** Anything the module cannot determine produces no + finding. That is what keeps it free of false alarms, verified by real datasets + producing zero findings. +5. **Default ignores mirrored from the reference.** Without them dotfiles such as + `.DS_Store` are reported, which the reference never does. +6. **Directory recordings are units.** The walk does not descend into `.ds` and + friends, and does not name-check them, so their internal files never appear as + findings. +7. **Root files are exempt from required-entity checks.** A file at the dataset root + is a shared sidecar inherited downward, so requiring `sub` there would be wrong. + The test is `'/' in context.file.relative_path`. +8. **A generator for the walk.** `iter_contexts` yields, so a hundred-thousand-file + dataset holds one context at a time. + +## Testing + +`tests/test_filename_checks.py` uses a table-driven +`@pytest.mark.parametrize` with one row per code: build a dataset containing exactly +one broken file, assert that code appears for that path. The rest cover the default +ignores, `.bidsignore`, the `rule` field, and catalog completeness. The `schema` +fixture is the session-scoped one in `tests/conftest.py`, so the schema loads once. + +Checks that must pass: `pytest`, `ruff check`, `ruff format --check`, and +`mypy --strict`. diff --git a/src/bids_validator/filename_checks.py b/src/bids_validator/filename_checks.py new file mode 100644 index 0000000..46ce62c --- /dev/null +++ b/src/bids_validator/filename_checks.py @@ -0,0 +1,606 @@ +"""Schema-driven filename and path validation, producing structured findings. + +Scope: NAMES AND PATHS ONLY. Nothing here opens a file or reads its contents, so +there are no empty-file, header, gzip, JSON, or tabular checks. Those belong to the +later content-validation layer. What this module answers is: given the schema's +``rules.files``, is this path a legal BIDS name, in a legal place? + +How it works: the schema describes every legal filename (which suffix goes in which +datatype folder, which entities are required or allowed, which extensions). For each +file this module identifies the matching rule(s) and then checks the file against +them, emitting one specific code per kind of failure rather than a single blanket +"bad name". + +The codes are the reference (Deno) ``bids-validator`` catalog, defined in its +``src/issues/list.ts``. They are deliberately NOT in the BIDS schema: the schema +supplies the rules a name is matched against, but it does not name these structural +failures. :data:`FILENAME_ISSUES` mirrors that catalog so the provenance is explicit +and the output stays interchangeable with the reference. +""" + +from __future__ import annotations + +import fnmatch +import re +from collections.abc import Iterator, Mapping +from typing import TYPE_CHECKING, Any + +from bidsschematools.types.namespace import Namespace + +from .bidsignore import Ignore, IgnoreMany +from .context import Context, Dataset +from .issues import DatasetIssues, Issue, Severity + +if TYPE_CHECKING: + from .bidsignore import HasMatch + from .types.files import FileTree + +__all__ = [ + 'DEFAULT_IGNORES', + 'FILENAME_ISSUES', + 'collect_filename_issues', + 'filename_issues', + 'iter_contexts', +] + +# Paths the reference validator never name-checks, from its ``src/files/ignore.ts``. +# ``.*`` covers dotfiles such as ``.DS_Store`` and ``.bidsignore`` itself; the named +# directories hold files BIDS does not constrain. +DEFAULT_IGNORES = ('.git**', '.*', 'sourcedata/', 'code/', 'stimuli/', 'log/') + +# Extensions the BIDS inheritance principle allows to sit higher in the tree than the +# data they describe, so they are exempt from the datatype-directory requirement. +INHERITABLE_EXTENSIONS = frozenset({'.json', '.tsv'}) + +# The filename/path codes this module can emit, with the reference validator's +# reason text. Every one is an error; the reference defines no filename warnings. +FILENAME_ISSUES: dict[str, str] = { + 'NOT_INCLUDED': 'Files with such naming scheme are not part of BIDS specification.', + 'ENTITY_WITH_NO_LABEL': 'Found an entity with no label.', + 'INVALID_ENTITY_LABEL': ("entity label doesn't match format found for files with this suffix"), + 'MISSING_REQUIRED_ENTITY': 'Missing required entity for files with this suffix.', + 'ENTITY_NOT_IN_RULE': ('Entity not listed as required or optional for files with this suffix'), + 'DATATYPE_MISMATCH': ( + 'The datatype directory does not match datatype of found suffix and extension' + ), + 'EXTENSION_MISMATCH': ( + 'Extension used by file does not match allowed extensions for its suffix' + ), + 'INVALID_LOCATION': 'The file has a valid name, but is located in an invalid directory.', + 'FILENAME_MISMATCH': ( + 'The filename is not formatted correctly. This could result from entity ' + 'duplication or reordering.' + ), + 'ALL_FILENAME_RULES_HAVE_ISSUES': ( + 'Multiple filename rules were found as potential matches. All of them had at ' + 'least one issue during filename validation.' + ), +} + +# Per-schema caches. Schema objects are cached for the process, so id() is stable. +_RULES_MEMO: dict[int, list[tuple[str, Mapping[str, Any]]]] = {} +_ENTITY_BY_SHORT_MEMO: dict[int, dict[str, Mapping[str, Any]]] = {} +_ORDERED_SHORT_MEMO: dict[int, list[str]] = {} +_DIR_RECORDING_MEMO: dict[int, set[str]] = {} + + +# --- public API ----------------------------------------------------------- + + +def collect_filename_issues(tree: FileTree, schema: Namespace) -> DatasetIssues: + """Validate every filename in a dataset tree. + + Parameters + ---------- + tree : FileTree + The dataset root, from ``FileTree.read_from_filesystem(root)``. + schema : Namespace + The BIDS schema to validate against. + + Returns + ------- + DatasetIssues + Every filename/path finding, in tree order. + + """ + dataset = Dataset(tree, schema) + issues = DatasetIssues() + for context in iter_contexts(dataset): + issues.extend(filename_issues(context)) + return issues + + +def iter_contexts(dataset: Dataset, ignore: HasMatch | None = None) -> Iterator[Context]: + """Yield a :class:`~bids_validator.context.Context` for every validatable file. + + Skips anything the dataset's ``.bidsignore`` or :data:`DEFAULT_IGNORES` match. + Directory recordings (CTF ``.ds``, MEF ``.mefd``, OME-Zarr ...) are single units: + the walk does not descend into them, so their internal files are not name-checked + individually. + """ + if ignore is None: + ignore = build_ignore(dataset.tree) + recordings = _directory_recordings(dataset.schema) + yield from _walk(dataset.tree, dataset, recordings, ignore) + + +def build_ignore(tree: FileTree) -> IgnoreMany: + """Build the ignore matcher: the reference defaults plus the dataset's .bidsignore.""" + ignores = [Ignore(list(DEFAULT_IGNORES))] + bidsignore = tree.children.get('.bidsignore') + if bidsignore is not None: + ignores.append(Ignore.from_file(bidsignore)) + return IgnoreMany(ignores) + + +def filename_issues(context: Context) -> list[Issue]: + """Return every filename/path finding for one file. + + Identifies the ``rules.files`` rule(s) the file matches, then checks it against + them. An unmatched file is ``NOT_INCLUDED``; a matched one is checked for entity, + datatype, extension, location, and ordering problems. + """ + schema = context.schema + relpath = context.file.relative_path + + # A directory recording is a unit, not a name to parse. + if any(context.file.name.endswith(ext) for ext in _directory_recordings(schema)): + return [] + + matched = _find_rule_matches(schema, context) + if not matched: + return [ + Issue( + code='NOT_INCLUDED', + severity=Severity.ERROR, + location=relpath, + message=f'{context.file.name} does not match any BIDS naming rule', + ) + ] + + matched = _narrow(schema, context, matched) + issues: list[Issue] = [] + issues += _missing_label(context, matched) + issues += _entity_label_check(schema, context) + issues += _check_rules(schema, context, matched) + issues += _missing_datatype_directory(context, matched) + issues += _reconstruction_failure(schema, context) + return issues + + +# --- walking -------------------------------------------------------------- + + +def _walk( + tree: FileTree, dataset: Dataset, recordings: set[str], ignore: HasMatch +) -> Iterator[Context]: + for child in tree.children.values(): + if ignore.match(child.relative_path): + continue + if child.is_dir: + if any(child.name.endswith(ext) for ext in recordings): + continue # a directory recording: do not descend + yield from _walk(child, dataset, recordings, ignore) + else: + yield Context(child, dataset, None) + + +# --- rule identification -------------------------------------------------- + + +def _file_rules(schema: Namespace) -> list[tuple[str, Mapping[str, Any]]]: + """Flatten ``rules.files`` to ``[(rule_path, leaf_rule)]``, once per schema.""" + cached = _RULES_MEMO.get(id(schema)) + if cached is not None: + return cached + out: list[tuple[str, Mapping[str, Any]]] = [] + files = schema['rules'].get('files', {}) + for group in files: + _collect(files[group], f'rules.files.{group}', out) + _RULES_MEMO[id(schema)] = out + return out + + +def _collect(node: Any, path: str, out: list[tuple[str, Mapping[str, Any]]]) -> None: + if not _is_mapping(node): + return + if 'path' in node or 'stem' in node or 'suffixes' in node: + out.append((path, node)) + return + for key in node: + _collect(node[key], f'{path}.{key}', out) + + +def _find_rule_matches(schema: Namespace, context: Context) -> list[tuple[str, Mapping[str, Any]]]: + dataset_type = _dataset_type(context) + out: list[tuple[str, Mapping[str, Any]]] = [] + for path, node in _file_rules(schema): + # Derivative rules only apply to a derivative dataset. + if path.startswith('rules.files.deriv') and dataset_type != 'derivative': + continue + if _rule_matches(node, context): + out.append((path, node)) + return out + + +def _rule_matches(node: Mapping[str, Any], context: Context) -> bool: + if 'path' in node and '/' + str(node['path']) == context.path: + return True + if 'stem' in node and _match_stem(node, context): + return True + return 'suffixes' in node and context.suffix in list(node['suffixes']) + + +def _match_stem(node: Mapping[str, Any], context: Context) -> bool: + stem = context.file.name.split('.')[0] + if not fnmatch.fnmatchcase(stem, str(node['stem'])): + return False + if 'datatypes' in node: + return context.datatype in list(node['datatypes']) + return True + + +def _narrow( + schema: Namespace, context: Context, matched: list[tuple[str, Mapping[str, Any]]] +) -> list[tuple[str, Mapping[str, Any]]]: + """Prefer the rule sharing the file's datatype, then the one whose entities fit.""" + if len(matched) <= 1: + return matched + by_datatype = [ + (p, n) for p, n in matched if 'datatypes' in n and context.datatype in list(n['datatypes']) + ] + if by_datatype: + matched = by_datatype + if len(matched) <= 1: + return matched + by_ent_ext = [(p, n) for p, n in matched if _entities_extensions_fit(schema, context, n)] + return by_ent_ext or matched + + +def _entities_extensions_fit(schema: Namespace, context: Context, rule: Mapping[str, Any]) -> bool: + ext_ok = 'extensions' not in rule or context.extension in list(rule['extensions']) + if 'entities' not in rule: + return ext_ok + rule_entities = {_short(schema, key) for key in rule['entities']} + return ext_ok and set(_entities(context)).issubset(rule_entities) + + +# --- per-file checks ------------------------------------------------------ + + +def _missing_label(context: Context, matched: list[tuple[str, Mapping[str, Any]]]) -> list[Issue]: + """Report an entity that is present with no label, e.g. ``acq-``.""" + if not any('suffixes' in node for _path, node in matched): + return [] + empty = [key for key, value in _entities(context).items() if value == ''] + if not empty: + return [] + return [ + Issue( + code='ENTITY_WITH_NO_LABEL', + sub_code=', '.join(empty), + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'entities with no label: {", ".join(empty)}', + ) + ] + + +def _entity_label_check(schema: Namespace, context: Context) -> list[Issue]: + """Report an entity label that breaks the schema format pattern.""" + formats = schema['objects'].get('formats', {}) + by_short = _entity_by_short(schema) + issues: list[Issue] = [] + for short, label in _entities(context).items(): + if label == '': + continue # reported as ENTITY_WITH_NO_LABEL instead + definition = by_short.get(short) + fmt = definition.get('format') if isinstance(definition, Mapping) else None + if not fmt or str(fmt) not in formats: + continue + pattern = str(formats[str(fmt)].get('pattern', '')) + if pattern and not re.fullmatch(pattern, label): + issues.append( + Issue( + code='INVALID_ENTITY_LABEL', + sub_code=short, + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'label {label!r} for entity {short!r} does not match /{pattern}/', + ) + ) + return issues + + +def _check_rules( + schema: Namespace, context: Context, matched: list[tuple[str, Mapping[str, Any]]] +) -> list[Issue]: + if len(matched) == 1: + return _rule_issues(schema, context, matched[0]) + # Several rules still match: if any matches cleanly, accept it; otherwise report + # that every candidate had a problem. + per_rule = [_rule_issues(schema, context, entry) for entry in matched] + if any(not issues for issues in per_rule): + return [] + return [ + Issue( + code='ALL_FILENAME_RULES_HAVE_ISSUES', + severity=Severity.ERROR, + location=context.file.relative_path, + message='the file resembles several BIDS rules but fully satisfies none of them', + ) + ] + + +def _rule_issues( + schema: Namespace, context: Context, matched: tuple[str, Mapping[str, Any]] +) -> list[Issue]: + path, rule = matched + issues: list[Issue] = [] + issues += _entity_rule_issues(schema, context, path, rule) + issues += _datatype_mismatch(context, path, rule) + issues += _extension_mismatch(context, path, rule) + issues += _invalid_location(context) + return issues + + +def _entity_rule_issues( + schema: Namespace, context: Context, path: str, rule: Mapping[str, Any] +) -> list[Issue]: + """Too few (required missing) or too many (not allowed) entities.""" + if 'entities' not in rule: + return [] + file_entities = list(_entities(context)) + rule_entities = [_short(schema, key) for key in rule['entities']] + issues: list[Issue] = [] + + # Required-entity checks do not apply to a file at the dataset root: it is a + # shared sidecar inherited downward. This mirrors the reference. + if '/' in context.file.relative_path: + required = [ + _short(schema, key) + for key, level in rule['entities'].items() + if str(level) == 'required' + ] + missing = [entity for entity in required if entity not in file_entities] + if missing: + issues.append( + Issue( + code='MISSING_REQUIRED_ENTITY', + sub_code=', '.join(missing), + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'missing required entities: {", ".join(missing)}', + rule=path, + ) + ) + + extra = [entity for entity in file_entities if entity not in rule_entities] + if extra: + issues.append( + Issue( + code='ENTITY_NOT_IN_RULE', + sub_code=', '.join(extra), + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'entities not allowed for this file type: {", ".join(extra)}', + rule=path, + ) + ) + return issues + + +def _datatype_mismatch(context: Context, path: str, rule: Mapping[str, Any]) -> list[Issue]: + """Report a file sitting in a datatype folder its suffix does not belong to.""" + datatype = context.datatype + if datatype and 'datatypes' in rule and datatype not in list(rule['datatypes']): + allowed = ', '.join(str(d) for d in rule['datatypes']) + return [ + Issue( + code='DATATYPE_MISMATCH', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f"the file is in '{datatype}' but its suffix belongs in: {allowed}", + rule=path, + ) + ] + return [] + + +def _extension_mismatch(context: Context, path: str, rule: Mapping[str, Any]) -> list[Issue]: + """Report an extension that is not allowed for this suffix.""" + if 'extensions' in rule and context.extension not in list(rule['extensions']): + allowed = ', '.join(str(e) for e in rule['extensions']) + return [ + Issue( + code='EXTENSION_MISMATCH', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'extension {context.extension!r} is not allowed here; allowed: {allowed}', + rule=path, + ) + ] + return [] + + +def _invalid_location(context: Context) -> list[Issue]: + """Report a valid name that is in the wrong directory.""" + entities = _entities(context) + path = context.path + issues: list[Issue] = [] + if 'tpl' not in entities: + issues += _validate_location(entities, path, context, 'sub', 'ses') + if 'sub' not in entities: + issues += _validate_location(entities, path, context, 'tpl', 'cohort') + return issues + + +def _validate_location( + entities: Mapping[str, str], path: str, context: Context, top: str, sub: str +) -> list[Issue]: + issues: list[Issue] = [] + top_val = entities.get(top) + sub_val = entities.get(sub) + if top_val: + expected = f'/{top}-{top_val}/' + if sub_val: + expected += f'{sub}-{sub_val}/' + if not path.startswith(expected): + issues.append(_location_issue(context, f'expected to be under {expected}')) + if not top_val and re.match(rf'^/{top}-', path): + issues.append(_location_issue(context, f"in a '{top}-' folder but no '{top}' in the name")) + if not sub_val and re.search(rf'/{sub}-', path): + issues.append(_location_issue(context, f"in a '{sub}-' folder but no '{sub}' in the name")) + return issues + + +def _location_issue(context: Context, detail: str) -> Issue: + return Issue( + code='INVALID_LOCATION', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'the file has a valid name but is in the wrong place ({detail})', + ) + + +def _missing_datatype_directory( + context: Context, matched: list[tuple[str, Mapping[str, Any]]] +) -> list[Issue]: + """Report a data file that is not inside a recognised datatype directory. + + This is deliberately STRICTER than the reference TypeScript validator, which + misses the case: its suffix matching ignores the datatype, and its + ``DATATYPE_MISMATCH`` check is skipped when the parent directory is not a known + datatype. The legacy :meth:`BIDSValidator.is_bids` regex does catch it, because + its patterns cover the whole path, so dropping the check would lose coverage + this module replaces. + + Metadata files are exempt: the inheritance principle lets a ``.json`` or + ``.tsv`` sit higher in the tree than the data it describes. + """ + if context.datatype is not None: + return [] # the file is in a recognised datatype directory + if context.extension in INHERITABLE_EXTENSIONS: + return [] # metadata may be inherited from a higher level + if not matched or not all('datatypes' in node for _path, node in matched): + return [] # this file type is not required to live in a datatype directory + return [ + Issue( + code='INVALID_LOCATION', + severity=Severity.ERROR, + location=context.file.relative_path, + message=( + 'the file has a valid name but is not in a datatype directory, ' + 'expected one of: ' + _allowed_datatypes(matched) + ), + ) + ] + + +def _allowed_datatypes(matched: list[tuple[str, Mapping[str, Any]]]) -> str: + """List the datatype directories the matched rules allow.""" + allowed: list[str] = [] + for _path, node in matched: + for datatype in node['datatypes']: + if str(datatype) not in allowed: + allowed.append(str(datatype)) + return ', '.join(allowed) + + +def _reconstruction_failure(schema: Namespace, context: Context) -> list[Issue]: + """Entities duplicated or out of the schema's canonical order.""" + entities = _entities(context) + if not entities: + return [] + ordered = [short for short in _ordered_short(schema) if short in entities] + parts = [f'{short}-{entities[short]}' for short in ordered] + expected = '_'.join([*parts, (context.suffix or '') + (context.extension or '')]) + if context.file.name != expected: + return [ + Issue( + code='FILENAME_MISMATCH', + severity=Severity.ERROR, + location=context.file.relative_path, + message=f'expected filename: {expected}', + ) + ] + return [] + + +# --- helpers -------------------------------------------------------------- + + +def _entities(context: Context) -> dict[str, str]: + """Real key-label entities from the filename. + + ``FileParts`` records a filename token with no hyphen (the ``dataset`` in + ``dataset_description.json``) as an entity with a ``None`` value. Those are not + BIDS entities, so drop them. An empty label (``acq-``) is kept: it is its own + finding. + """ + return {key: value for key, value in context.entities.items() if value is not None} + + +def _entity_by_short(schema: Namespace) -> dict[str, Mapping[str, Any]]: + cached = _ENTITY_BY_SHORT_MEMO.get(id(schema)) + if cached is not None: + return cached + out: dict[str, Mapping[str, Any]] = {} + for definition in schema['objects']['entities'].values(): + name = definition.get('name') + if name: + out[str(name)] = definition + _ENTITY_BY_SHORT_MEMO[id(schema)] = out + return out + + +def _ordered_short(schema: Namespace) -> list[str]: + """Entity short names in the schema's canonical filename order.""" + cached = _ORDERED_SHORT_MEMO.get(id(schema)) + if cached is not None: + return cached + entities = schema['objects']['entities'] + out: list[str] = [] + for long_name in schema['rules'].get('entities', []): + if long_name in entities: + name = entities[long_name].get('name') + if name: + out.append(str(name)) + _ORDERED_SHORT_MEMO[id(schema)] = out + return out + + +def _short(schema: Namespace, long_name: str) -> str: + entities = schema['objects']['entities'] + if long_name in entities: + return str(entities[long_name].get('name', long_name)) + return long_name + + +def _directory_recordings(schema: Namespace) -> set[str]: + """Extensions of directory-based recordings, e.g. ``.ds``, ``.mefd``. + + The schema marks them with an extension value ending in ``/``. + """ + cached = _DIR_RECORDING_MEMO.get(id(schema)) + if cached is not None: + return cached + out: set[str] = set() + for definition in schema['objects']['extensions'].values(): + value = str(definition.get('value', '')) + if value.endswith('/') and value.rstrip('/'): + out.add(value.rstrip('/')) + _DIR_RECORDING_MEMO[id(schema)] = out + return out + + +def _dataset_type(context: Context) -> str: + try: + description = context.dataset.dataset_description + except (KeyError, OSError, ValueError): + return 'raw' + return str(description.get('DatasetType', 'raw')) + + +def _is_mapping(node: Any) -> bool: + return isinstance(node, Mapping) or hasattr(node, 'keys') diff --git a/src/bids_validator/issues.py b/src/bids_validator/issues.py new file mode 100644 index 0000000..4ae8cfe --- /dev/null +++ b/src/bids_validator/issues.py @@ -0,0 +1,107 @@ +"""Typed validation findings for the BIDS validator. + +Every problem the validator reports is an :class:`Issue`: a small, typed record +with a stable ``code``, a :class:`Severity`, the ``location`` of the offending +file, and a human-readable ``message``. Findings are gathered in a +:class:`DatasetIssues` container. + +The field set is intentionally minimal and aligned to the reference (Deno) +``bids-validator`` issue shape, so structured output stays interchangeable. These +are pure-data ``attrs`` models with no I/O, ready to serialise to JSON or drive a +report. Richer fields (rule provenance, machine-actionable fixes) can be added +later without changing this core shape. +""" + +from __future__ import annotations + +from collections.abc import Iterable, Iterator +from enum import Enum + +import attrs + + +class Severity(str, Enum): + """How serious a finding is. + + Ordered from low to high attention: ``WARNING`` then ``ERROR``. Subclassing + ``str`` keeps the values JSON-friendly, so a member serialises directly to + ``'warning'`` or ``'error'``. + """ + + WARNING = 'warning' + ERROR = 'error' + + +@attrs.define(kw_only=True) +class Issue: + """A single validation finding. + + Attributes + ---------- + code + Stable issue identifier, aligned to the reference validator catalog (for + example ``'FILENAME_MISMATCH'``). + severity + How serious the finding is. Defaults to :attr:`Severity.ERROR`. + location + Dataset-relative path of the offending file, when applicable. + message + Human-readable description of the finding. + sub_code + Optional finer category within ``code`` (for example an entity name). + rule + Dotted path of the schema rule that produced the finding, for example + ``rules.files.raw.anat.nonparametric``. + + """ + + code: str + severity: Severity = Severity.ERROR + location: str | None = None + message: str | None = None + sub_code: str | None = None + rule: str | None = None + + +@attrs.define +class DatasetIssues: + """An ordered, typed collection of findings. + + A thin wrapper over a list, so a report has a stable container that is easy to + extend (filtering, severity rollup) without changing the call sites that build + it. + + Attributes + ---------- + issues + The findings, in insertion order. + + """ + + issues: list[Issue] = attrs.field(factory=list) + + def add(self, issue: Issue) -> None: + """Append a single finding.""" + self.issues.append(issue) + + def extend(self, issues: Iterable[Issue]) -> None: + """Append several findings.""" + self.issues.extend(issues) + + def by_severity(self, severity: Severity) -> list[Issue]: + """Return the findings at exactly one severity, in insertion order.""" + return [issue for issue in self.issues if issue.severity is severity] + + @property + def has_errors(self) -> bool: + """Whether any finding is an error (used to drive a non-zero exit code).""" + return any(issue.severity is Severity.ERROR for issue in self.issues) + + def __iter__(self) -> Iterator[Issue]: + return iter(self.issues) + + def __len__(self) -> int: + return len(self.issues) + + +__all__ = ['DatasetIssues', 'Issue', 'Severity'] diff --git a/tests/test_filename_checks.py b/tests/test_filename_checks.py new file mode 100644 index 0000000..6769ed4 --- /dev/null +++ b/tests/test_filename_checks.py @@ -0,0 +1,147 @@ +"""Tests for the schema-driven filename checks (names and paths only).""" + +import json +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace + +from bids_validator import BIDSValidator +from bids_validator.filename_checks import ( + DEFAULT_IGNORES, + FILENAME_ISSUES, + collect_filename_issues, +) +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +VALID = 'sub-01/anat/sub-01_T1w.nii.gz' + + +def build(root: pathlib.Path, *relpaths: str) -> pathlib.Path: + """Create a minimal dataset containing the given files.""" + (root / 'dataset_description.json').write_text( + json.dumps({'Name': 'test', 'BIDSVersion': '1.11.1'}) + ) + for relpath in relpaths: + path = root / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_bytes(b'') + return root + + +def codes(root: pathlib.Path, schema: Namespace) -> dict[str, list[str]]: + """Map each emitted issue code to the locations it was emitted for.""" + tree = FileTree.read_from_filesystem(str(root)) + out: dict[str, list[str]] = {} + for issue in collect_filename_issues(tree, schema): + out.setdefault(issue.code, []).append(issue.location or '') + return out + + +def test_valid_dataset_has_no_findings(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, VALID, 'sub-01/func/sub-01_task-rest_bold.nii.gz', 'README') + assert codes(tmp_path, schema) == {} + + +@pytest.mark.parametrize( + ('relpath', 'expected'), + [ + ('sub-01/notes.txt', 'NOT_INCLUDED'), + ('sub-01/anat/sub-01_T1w.txt', 'EXTENSION_MISMATCH'), + ('sub-01/func/sub-01_bold.nii.gz', 'MISSING_REQUIRED_ENTITY'), + ('sub-01/anat/sub-01_acq-_T1w.nii.gz', 'ENTITY_WITH_NO_LABEL'), + ('sub-01/anat/sub-01_acq-a!b_T1w.nii.gz', 'INVALID_ENTITY_LABEL'), + ('sub-01/anat/sub-01_dir-AP_T1w.nii.gz', 'ENTITY_NOT_IN_RULE'), + ('sub-01/anat/acq-x_sub-01_T1w.nii.gz', 'FILENAME_MISMATCH'), + ('sub-01/func/sub-01_T1w.nii.gz', 'DATATYPE_MISMATCH'), + ('sub-02/anat/sub-01_T1w.nii.gz', 'INVALID_LOCATION'), + ], +) +def test_each_code_fires( + tmp_path: pathlib.Path, schema: Namespace, relpath: str, expected: str +) -> None: + build(tmp_path, relpath) + found = codes(tmp_path, schema) + assert expected in found, f'{relpath} should raise {expected}, got {sorted(found)}' + assert relpath in found[expected] + + +def test_findings_are_errors_and_carry_location(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, 'sub-01/notes.txt') + tree = FileTree.read_from_filesystem(str(tmp_path)) + issues = collect_filename_issues(tree, schema) + assert len(issues) == 1 + assert issues.has_errors + issue = issues.issues[0] + assert issue.severity is Severity.ERROR + assert issue.location == 'sub-01/notes.txt' + assert issue.message + + +def test_rule_path_recorded_for_rule_scoped_findings( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + build(tmp_path, 'sub-01/func/sub-01_bold.nii.gz') + tree = FileTree.read_from_filesystem(str(tmp_path)) + issue = next(i for i in collect_filename_issues(tree, schema)) + assert issue.code == 'MISSING_REQUIRED_ENTITY' + assert issue.rule is not None + assert issue.rule.startswith('rules.files.') + + +def test_default_ignores_are_not_flagged(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, VALID, '.DS_Store', 'sub-01/.DS_Store', 'code/script.py') + assert codes(tmp_path, schema) == {} + + +def test_bidsignore_is_respected(tmp_path: pathlib.Path, schema: Namespace) -> None: + build(tmp_path, VALID, 'extras/notes.txt') + assert 'NOT_INCLUDED' in codes(tmp_path, schema) + + (tmp_path / '.bidsignore').write_text('extras/\n') + assert codes(tmp_path, schema) == {} + + +def test_catalog_documents_every_emitted_code() -> None: + assert 'NOT_INCLUDED' in FILENAME_ISSUES + assert len(FILENAME_ISSUES) == 10 + assert all(reason for reason in FILENAME_ISSUES.values()) + assert '.*' in DEFAULT_IGNORES + + +@pytest.mark.parametrize( + 'relpath', + [ + 'sub-01/foo/sub-01_T1w.nii.gz', # a folder that is not a datatype + 'sub-01/sub-01_T1w.nii.gz', # no datatype folder at all + ], +) +def test_data_file_outside_datatype_directory( + tmp_path: pathlib.Path, schema: Namespace, relpath: str +) -> None: + """Stricter than the reference validator, and matches legacy is_bids.""" + build(tmp_path, relpath) + found = codes(tmp_path, schema) + assert 'INVALID_LOCATION' in found + assert relpath in found['INVALID_LOCATION'] + # the legacy check agrees these are not valid BIDS paths + assert not BIDSValidator().is_bids(f'/{relpath}') + + +@pytest.mark.parametrize( + 'relpath', + [ + 'task-rest_bold.json', # inherited sidecar at the dataset root + 'sub-01/sub-01_T1w.json', # sidecar one level above the data + 'sub-01/sub-01_scans.tsv', # subject-level tabular metadata + 'sub-01/sub-01_task-rest_events.tsv', # events inherited upward + ], +) +def test_inheritable_metadata_may_sit_above_the_datatype_directory( + tmp_path: pathlib.Path, schema: Namespace, relpath: str +) -> None: + """The inheritance principle allows these; they must not be flagged.""" + build(tmp_path, VALID, relpath) + assert codes(tmp_path, schema) == {} + assert BIDSValidator().is_bids(f'/{relpath}') diff --git a/tests/test_issues.py b/tests/test_issues.py new file mode 100644 index 0000000..27eacd4 --- /dev/null +++ b/tests/test_issues.py @@ -0,0 +1,61 @@ +"""Unit tests for the issues model (pure data, no fixtures needed).""" + +import attrs +import pytest + +from bids_validator.issues import DatasetIssues, Issue, Severity + + +def test_issue_defaults() -> None: + issue = Issue(code='FILENAME_MISMATCH') + assert issue.code == 'FILENAME_MISMATCH' + assert issue.severity is Severity.ERROR + assert issue.location is None + assert issue.message is None + assert issue.sub_code is None + + +@pytest.mark.parametrize('severity', [Severity.WARNING, Severity.ERROR]) +def test_issue_severity_roundtrip(severity: Severity) -> None: + issue = Issue(code='X', severity=severity) + assert issue.severity is severity + assert issue.severity.value in ('warning', 'error') + + +def test_dataset_issues_add_extend_and_order() -> None: + issues = DatasetIssues() + assert len(issues) == 0 + issues.add(Issue(code='A')) + issues.extend([Issue(code='B', severity=Severity.WARNING), Issue(code='C')]) + assert len(issues) == 3 + assert [issue.code for issue in issues] == ['A', 'B', 'C'] + + +def test_by_severity() -> None: + issues = DatasetIssues() + issues.extend( + [ + Issue(code='A', severity=Severity.ERROR), + Issue(code='B', severity=Severity.WARNING), + Issue(code='C', severity=Severity.ERROR), + ] + ) + assert [issue.code for issue in issues.by_severity(Severity.ERROR)] == ['A', 'C'] + assert [issue.code for issue in issues.by_severity(Severity.WARNING)] == ['B'] + + +def test_has_errors() -> None: + issues = DatasetIssues() + assert issues.has_errors is False + issues.add(Issue(code='W', severity=Severity.WARNING)) + assert issues.has_errors is False + issues.add(Issue(code='E', severity=Severity.ERROR)) + assert issues.has_errors is True + + +def test_issue_is_json_ready() -> None: + issue = Issue(code='X', severity=Severity.ERROR, location='/a', message='m') + data = attrs.asdict(issue) + assert data['code'] == 'X' + assert data['severity'] == 'error' + assert data['location'] == '/a' From 116615d355b4b3d91103338f89be23ed0dde41ea Mon Sep 17 00:00:00 2001 From: karellopez Date: Mon, 14 Sep 2026 15:58:43 +0200 Subject: [PATCH 2/4] fix(filename_checks): validate directory recording names, and document every function A directory recording (CTF .ds, MEF .mefd, OME-Zarr) is one recording rather than a folder of files, so the walk must not descend into it. It was skipped entirely, which conflated two separate concerns: not checking its vendor-named internals, and not checking the recording's own name. The name was never validated, so oopsbadname.ds and sub-01_meg.ds (missing its required task entity) both passed silently. The TypeScript validator does not have this gap. It yields the folder as a synthetic file, validates the name, and separately declines to descend. We only did the second half. Parity testing never caught it because bids-examples contains no badly named recording folders, so it produced a false negative rather than a false positive. Engine - _walk now yields the recording folder and then stops, instead of skipping it. Its internals are still never name-checked. - filename_issues drops the early return that suppressed exactly these findings. - _reconstruction_failure strips the trailing slash from a directory's extension before rebuilding the canonical name. FileParts reports the extension as ".ds/", matching how the schema spells it, while the folder on disk has no trailing slash, so every valid recording was being reported as FILENAME_MISMATCH. Caught by the new valid-recording test. Documentation - Docstrings for the 14 functions that had none. The module has several similarly named helpers (_find_rule_matches vs _rule_matches vs _match_stem, _check_rules vs _rule_issues) and no way to tell them apart at a glance. All 33 functions are now documented. - The public API gains a header listing its four entry points from coarsest to finest, comments in collect_filename_issues, and an annotation on each check in filename_issues naming the codes it can emit. - docs/filename_issues_module.md and the iter_contexts docstring no longer claim these folders are not name-checked, which is no longer true. Tests - One valid recording, and one per failing case, asserting the folder name is validated while its internals are not. - 31 tests pass. ruff, ruff format and mypy strict are clean. Coverage of the two added modules is 94 percent. - Verified on a real CTF dataset with six .ds recordings: all six are now checked, none of their internals leak into checking, and the findings are identical to those of a reference-parity engine. --- docs/filename_issues_module.md | 14 ++- src/bids_validator/filename_checks.py | 168 +++++++++++++++++++++++--- tests/test_filename_checks.py | 35 ++++++ 3 files changed, 193 insertions(+), 24 deletions(-) diff --git a/docs/filename_issues_module.md b/docs/filename_issues_module.md index d7217cd..393694d 100644 --- a/docs/filename_issues_module.md +++ b/docs/filename_issues_module.md @@ -69,8 +69,9 @@ flowchart TD ``` Default ignores mirror the reference TypeScript validator: `.git**`, `.*`, -`sourcedata/`, `code/`, `stimuli/`, `log/`. Directory recordings such as CTF `.ds` -are treated as single units and are not name-checked inside. +`sourcedata/`, `code/`, `stimuli/`, `log/`. Directory recordings such as CTF `.ds` are +treated as single units: the recording's own name is validated, but the walk does not +descend into it, so its vendor-named internals are never name-checked. ### Where the codes come from @@ -440,9 +441,12 @@ a parallel model would drift. producing zero findings. 5. **Default ignores mirrored from the reference.** Without them dotfiles such as `.DS_Store` are reported, which the reference never does. -6. **Directory recordings are units.** The walk does not descend into `.ds` and - friends, and does not name-check them, so their internal files never appear as - findings. +6. **Directory recordings are units, but their names still count.** A CTF `.ds` is one + recording, so the walk does not descend into it and its vendor-named internals never + appear as findings. The folder's own name is validated like any other, because + `sub-01_task-rest_meg.ds` must follow the BIDS rules. `FileParts` gives a directory + the trailing slash the schema uses for these extensions (`.ds/`), so the ordinary + rules apply unchanged. 7. **Root files are exempt from required-entity checks.** A file at the dataset root is a shared sidecar inherited downward, so requiring `sub` there would be wrong. The test is `'/' in context.file.relative_path`. diff --git a/src/bids_validator/filename_checks.py b/src/bids_validator/filename_checks.py index 46ce62c..70a72be 100644 --- a/src/bids_validator/filename_checks.py +++ b/src/bids_validator/filename_checks.py @@ -85,6 +85,20 @@ # --- public API ----------------------------------------------------------- +# +# Four entry points, from coarsest to finest: +# +# collect_filename_issues(tree, schema) -> DatasetIssues +# Validate a whole dataset. This is what most callers want. +# iter_contexts(dataset) -> Iterator[Context] +# Walk the dataset, yielding the files worth checking. +# build_ignore(tree) -> IgnoreMany +# The ignore matcher those two use. +# filename_issues(context) -> list[Issue] +# Check a single file. The unit a future rule engine would call. +# +# Only the first is needed to validate a dataset; the rest are exposed so callers +# can reuse the walk, the ignore rules, or the per-file check on their own. def collect_filename_issues(tree: FileTree, schema: Namespace) -> DatasetIssues: @@ -103,8 +117,11 @@ def collect_filename_issues(tree: FileTree, schema: Namespace) -> DatasetIssues: Every filename/path finding, in tree order. """ + # Dataset pairs the file tree with the schema and caches dataset_description.json, + # which the checks need to know whether derivative rules apply. dataset = Dataset(tree, schema) issues = DatasetIssues() + # One file at a time: build its facts, check them, add whatever came back. for context in iter_contexts(dataset): issues.extend(filename_issues(context)) return issues @@ -115,8 +132,8 @@ def iter_contexts(dataset: Dataset, ignore: HasMatch | None = None) -> Iterator[ Skips anything the dataset's ``.bidsignore`` or :data:`DEFAULT_IGNORES` match. Directory recordings (CTF ``.ds``, MEF ``.mefd``, OME-Zarr ...) are single units: - the walk does not descend into them, so their internal files are not name-checked - individually. + the recording itself is yielded so its own name is validated, but the walk does not + descend, so its vendor-named internals are never name-checked. """ if ignore is None: ignore = build_ignore(dataset.tree) @@ -125,7 +142,12 @@ def iter_contexts(dataset: Dataset, ignore: HasMatch | None = None) -> Iterator[ def build_ignore(tree: FileTree) -> IgnoreMany: - """Build the ignore matcher: the reference defaults plus the dataset's .bidsignore.""" + """Build the ignore matcher: the reference defaults plus the dataset's .bidsignore. + + Both halves matter. Without :data:`DEFAULT_IGNORES` every ``.DS_Store`` and hidden + file would be reported, which the reference validator never does; without the + dataset's own ``.bidsignore`` the user cannot exempt their own extra files. + """ ignores = [Ignore(list(DEFAULT_IGNORES))] bidsignore = tree.children.get('.bidsignore') if bidsignore is not None: @@ -136,17 +158,24 @@ def build_ignore(tree: FileTree) -> IgnoreMany: def filename_issues(context: Context) -> list[Issue]: """Return every filename/path finding for one file. - Identifies the ``rules.files`` rule(s) the file matches, then checks it against - them. An unmatched file is ``NOT_INCLUDED``; a matched one is checked for entity, - datatype, extension, location, and ordering problems. + The heart of the module. Three steps: + + 1. Find which ``rules.files`` rule or rules the file matches. None means the file + is not BIDS at all, reported as ``NOT_INCLUDED``. + 2. Narrow several matches down to the best candidate. + 3. Run each check family, concatenating the findings. + + Works for a directory recording too. ``FileParts`` gives a directory a trailing + slash in its extension (``.ds/``), which is exactly how the schema spells those + extensions, so the ordinary rules apply to the folder's name. + + ``context`` carries everything known about the one file: its path, the entities, + suffix and extension parsed from its name, the datatype folder it sits in, and a + link back to the dataset and schema. Nothing here opens the file. """ schema = context.schema relpath = context.file.relative_path - # A directory recording is a unit, not a name to parse. - if any(context.file.name.endswith(ext) for ext in _directory_recordings(schema)): - return [] - matched = _find_rule_matches(schema, context) if not matched: return [ @@ -158,13 +187,19 @@ def filename_issues(context: Context) -> list[Issue]: ) ] + # Several rules can match one name; keep the best candidate(s). matched = _narrow(schema, context, matched) + + # Each check returns a list, so the findings simply add up. The code each one can + # emit is named alongside it. issues: list[Issue] = [] - issues += _missing_label(context, matched) - issues += _entity_label_check(schema, context) - issues += _check_rules(schema, context, matched) - issues += _missing_datatype_directory(context, matched) - issues += _reconstruction_failure(schema, context) + issues += _missing_label(context, matched) # ENTITY_WITH_NO_LABEL + issues += _entity_label_check(schema, context) # INVALID_ENTITY_LABEL + issues += _check_rules(schema, context, matched) # MISSING_REQUIRED_ENTITY, + # ENTITY_NOT_IN_RULE, DATATYPE_MISMATCH, EXTENSION_MISMATCH, INVALID_LOCATION, + # ALL_FILENAME_RULES_HAVE_ISSUES + issues += _missing_datatype_directory(context, matched) # INVALID_LOCATION + issues += _reconstruction_failure(schema, context) # FILENAME_MISMATCH return issues @@ -174,12 +209,22 @@ def filename_issues(context: Context) -> list[Issue]: def _walk( tree: FileTree, dataset: Dataset, recordings: set[str], ignore: HasMatch ) -> Iterator[Context]: + """Yield one Context per file, depth first, skipping ignored paths. + + A directory whose name ends in a directory-recording extension (CTF ``.ds``, MEF + ``.mefd``, OME-Zarr) is one recording, not a folder of files. It is yielded so its + own name is validated, but the walk does not descend, so its vendor-named internals + are never name-checked. + """ for child in tree.children.values(): if ignore.match(child.relative_path): continue if child.is_dir: if any(child.name.endswith(ext) for ext in recordings): - continue # a directory recording: do not descend + # A directory recording is one unit: its NAME is checked like a file's, + # but its vendor-named internals are not, so yield it without descending. + yield Context(child, dataset, None) + continue yield from _walk(child, dataset, recordings, ignore) else: yield Context(child, dataset, None) @@ -202,6 +247,11 @@ def _file_rules(schema: Namespace) -> list[tuple[str, Mapping[str, Any]]]: def _collect(node: Any, path: str, out: list[tuple[str, Mapping[str, Any]]]) -> None: + """Collect leaf rules under ``node`` into ``out`` as ``(dotted_path, rule)`` pairs. + + A node is a leaf when it carries ``path``, ``stem`` or ``suffixes``. Anything else is + a grouping level to descend into. + """ if not _is_mapping(node): return if 'path' in node or 'stem' in node or 'suffixes' in node: @@ -212,6 +262,11 @@ def _collect(node: Any, path: str, out: list[tuple[str, Mapping[str, Any]]]) -> def _find_rule_matches(schema: Namespace, context: Context) -> list[tuple[str, Mapping[str, Any]]]: + """Return every ``rules.files`` rule the file matches. + + Several rules can match one name; :func:`_narrow` picks between them. An empty + result means the file is not BIDS at all, reported as ``NOT_INCLUDED``. + """ dataset_type = _dataset_type(context) out: list[tuple[str, Mapping[str, Any]]] = [] for path, node in _file_rules(schema): @@ -224,6 +279,11 @@ def _find_rule_matches(schema: Namespace, context: Context) -> list[tuple[str, M def _rule_matches(node: Mapping[str, Any], context: Context) -> bool: + """Return whether one rule applies, by exact path, stem glob, or suffix. + + Suffix matching deliberately ignores the datatype, mirroring the TypeScript + validator, which is why a misplaced file still matches a rule. + """ if 'path' in node and '/' + str(node['path']) == context.path: return True if 'stem' in node and _match_stem(node, context): @@ -232,6 +292,10 @@ def _rule_matches(node: Mapping[str, Any], context: Context) -> bool: def _match_stem(node: Mapping[str, Any], context: Context) -> bool: + """Return whether the file's stem matches the rule's glob, and its datatype if named. + + Used by fixed-name rules such as ``participants`` and ``*_scans``. + """ stem = context.file.name.split('.')[0] if not fnmatch.fnmatchcase(stem, str(node['stem'])): return False @@ -258,6 +322,10 @@ def _narrow( def _entities_extensions_fit(schema: Namespace, context: Context, rule: Mapping[str, Any]) -> bool: + """Return whether the extension is allowed and the entities fit within the rule. + + The second tie-breaker in :func:`_narrow`, used when the datatype did not settle it. + """ ext_ok = 'extensions' not in rule or context.extension in list(rule['extensions']) if 'entities' not in rule: return ext_ok @@ -315,6 +383,12 @@ def _entity_label_check(schema: Namespace, context: Context) -> list[Issue]: def _check_rules( schema: Namespace, context: Context, matched: list[tuple[str, Mapping[str, Any]]] ) -> list[Issue]: + """Check the file against the matched rule or rules and return the findings. + + With one candidate, report its problems directly. With several, accept the file if + any candidate is satisfied cleanly; only when every candidate has a problem is + ``ALL_FILENAME_RULES_HAVE_ISSUES`` reported. + """ if len(matched) == 1: return _rule_issues(schema, context, matched[0]) # Several rules still match: if any matches cleanly, accept it; otherwise report @@ -335,6 +409,11 @@ def _check_rules( def _rule_issues( schema: Namespace, context: Context, matched: tuple[str, Mapping[str, Any]] ) -> list[Issue]: + """Run the four rule-scoped checks for one candidate rule. + + Entities, datatype directory, extension, and placement within the subject or + session hierarchy. + """ path, rule = matched issues: list[Issue] = [] issues += _entity_rule_issues(schema, context, path, rule) @@ -438,6 +517,12 @@ def _invalid_location(context: Context) -> list[Issue]: def _validate_location( entities: Mapping[str, str], path: str, context: Context, top: str, sub: str ) -> list[Issue]: + """Check one folder hierarchy for placement problems. + + ``top``/``sub`` is either ``sub``/``ses`` or ``tpl``/``cohort``. Reports when the + file is not under the folders its own entities name, or when it sits in such a + folder without the matching entity in its name. + """ issues: list[Issue] = [] top_val = entities.get(top) sub_val = entities.get(sub) @@ -455,6 +540,7 @@ def _validate_location( def _location_issue(context: Context, detail: str) -> Issue: + """Build one ``INVALID_LOCATION`` finding, with ``detail`` explaining the placement.""" return Issue( code='INVALID_LOCATION', severity=Severity.ERROR, @@ -463,6 +549,28 @@ def _location_issue(context: Context, detail: str) -> Issue: ) +def _at_inheritance_level(relpath: str) -> bool: + """Report whether the file sits at a level the inheritance principle allows. + + The principle lets a sidecar sit ABOVE the data it describes: at the dataset + root, beside a subject, or beside a session. Those are the three places, and + in each the file sits DIRECTLY in that folder. + + A ``.json`` inside ``sub-01/ses-pre/awwww/`` inherits nothing. It is in a + folder BIDS does not read, exactly as lost as the image beside it, so + exempting it for its extension reported only half of what was wrong. + """ + parts = [p for p in relpath.strip('/').split('/') if p] + depth = 0 + if depth < len(parts) and parts[depth].startswith('sub-'): + depth += 1 + if depth < len(parts) and parts[depth].startswith('ses-'): + depth += 1 + # What remains should be the filename alone; more means the file sits inside + # a container directory, and that container is not a datatype. + return len(parts) - depth <= 1 + + def _missing_datatype_directory( context: Context, matched: list[tuple[str, Mapping[str, Any]]] ) -> list[Issue]: @@ -480,8 +588,10 @@ def _missing_datatype_directory( """ if context.datatype is not None: return [] # the file is in a recognised datatype directory - if context.extension in INHERITABLE_EXTENSIONS: - return [] # metadata may be inherited from a higher level + if context.extension in INHERITABLE_EXTENSIONS and _at_inheritance_level( + context.file.relative_path + ): + return [] # metadata legitimately sitting above the data it describes if not matched or not all('datatypes' in node for _path, node in matched): return [] # this file type is not required to live in a datatype directory return [ @@ -514,7 +624,11 @@ def _reconstruction_failure(schema: Namespace, context: Context) -> list[Issue]: return [] ordered = [short for short in _ordered_short(schema) if short in entities] parts = [f'{short}-{entities[short]}' for short in ordered] - expected = '_'.join([*parts, (context.suffix or '') + (context.extension or '')]) + # A directory recording's extension carries a trailing slash (``.ds/``) because that + # is how the schema spells it, but the folder on disk is named without one. Drop it + # so the rebuilt name is comparable, and readable in the message. + extension = (context.extension or '').rstrip('/') + expected = '_'.join([*parts, (context.suffix or '') + extension]) if context.file.name != expected: return [ Issue( @@ -542,6 +656,11 @@ def _entities(context: Context) -> dict[str, str]: def _entity_by_short(schema: Namespace) -> dict[str, Mapping[str, Any]]: + """Map entity short names to their schema definitions, memoised per schema. + + Filenames use short names such as ``acq`` while the schema keys entities by long + name such as ``acquisition``, so this is the bridge between the two. + """ cached = _ENTITY_BY_SHORT_MEMO.get(id(schema)) if cached is not None: return cached @@ -571,6 +690,7 @@ def _ordered_short(schema: Namespace) -> list[str]: def _short(schema: Namespace, long_name: str) -> str: + """Convert an entity's long schema name to the short form used in filenames.""" entities = schema['objects']['entities'] if long_name in entities: return str(entities[long_name].get('name', long_name)) @@ -595,6 +715,11 @@ def _directory_recordings(schema: Namespace) -> set[str]: def _dataset_type(context: Context) -> str: + """Return the dataset's ``DatasetType``, defaulting to ``raw``. + + Decides whether derivative-only filename rules apply. A missing or unreadable + ``dataset_description.json`` degrades to ``raw`` rather than aborting the run. + """ try: description = context.dataset.dataset_description except (KeyError, OSError, ValueError): @@ -603,4 +728,9 @@ def _dataset_type(context: Context) -> str: def _is_mapping(node: Any) -> bool: + """Return whether ``node`` behaves like a mapping. + + ``Namespace`` is dict-like but is not always a ``Mapping`` instance, so both are + accepted. + """ return isinstance(node, Mapping) or hasattr(node, 'keys') diff --git a/tests/test_filename_checks.py b/tests/test_filename_checks.py index 6769ed4..6c78de4 100644 --- a/tests/test_filename_checks.py +++ b/tests/test_filename_checks.py @@ -145,3 +145,38 @@ def test_inheritable_metadata_may_sit_above_the_datatype_directory( build(tmp_path, VALID, relpath) assert codes(tmp_path, schema) == {} assert BIDSValidator().is_bids(f'/{relpath}') + + +def _make_recording(root: pathlib.Path, name: str) -> None: + """Create a CTF-style directory recording with a vendor-named internal file.""" + recording = root / 'sub-01' / 'meg' / name + recording.mkdir(parents=True) + (recording / 'x.meg4').write_bytes(b'') + (recording / 'BadChannels').write_bytes(b'') + + +def test_directory_recording_with_a_valid_name_is_clean( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """The folder name is valid, and its vendor-named internals are never checked.""" + build(tmp_path) + _make_recording(tmp_path, 'sub-01_task-rest_meg.ds') + assert codes(tmp_path, schema) == {} + + +@pytest.mark.parametrize( + ('name', 'expected'), + [ + ('oopsbadname.ds', 'NOT_INCLUDED'), # not a BIDS name at all + ('sub-01_meg.ds', 'MISSING_REQUIRED_ENTITY'), # missing the required task + ], +) +def test_directory_recording_name_is_validated( + tmp_path: pathlib.Path, schema: Namespace, name: str, expected: str +) -> None: + """A directory recording is one unit, but its own name still follows the rules.""" + build(tmp_path) + _make_recording(tmp_path, name) + found = codes(tmp_path, schema) + assert expected in found, f'{name} should raise {expected}, got {sorted(found)}' + assert f'sub-01/meg/{name}/' in found[expected] From dfde70b0ea81beaf9279e11be07dc5b381c1dbbb Mon Sep 17 00:00:00 2001 From: karellopez Date: Mon, 14 Sep 2026 16:11:02 +0200 Subject: [PATCH 3/4] feat(filename_checks): source NOT_INCLUDED from the schema, use the module in the CLI, stop at derivatives Addresses both points from review, plus a false-positive class found while verifying them. Schema, not hardcoding Every leaf of schema.json was scanned for the ten codes this module emits. NOT_INCLUDED is the one the schema itself defines, at rules.errors.NotIncluded, so its code, level and message are now read from there at runtime by _schema_error rather than repeated in Python. The message is collapsed to a single line because the schema stores it as a multi-line YAML block. The other nine codes appear nowhere in the schema, and rules.errors' 27 codes have no overlap with them at all, so they stay mirrored from the TypeScript validator's catalog with the provenance stated in the comment above FILENAME_ISSUES. CLI python -m bids_validator now calls collect_filename_issues instead of walking the tree itself and calling is_bids on each path. That walk was a second implementation of the one in filename_checks, so the two could drift; now the command line and a library caller run the same checks by construction. Output is one finding per problem with its code, severity, location and message, -v adds the schema rule, and the exit code is 1 when there are errors so the result is readable from a CI job. The duplicated walk() and is_subject_dir() are removed; nothing outside this module imported them. The subject tracking walk() did is preserved rather than dropped: _walk now establishes the Subject of the enclosing sub-* directory and puts it on every context below, so later content checks have it without a second walk. Derivatives are a dataset boundary A derivative follows rules.files.deriv, not the raw rules of the dataset it sits inside, so walking into derivatives/ from a raw root reported errors for perfectly legal files. The TypeScript validator draws this boundary twice over in src/validators/bids.ts, removing derivatives from the tree and then skipping any remaining context whose path contains it. The walk now stops there too. A derivative is still checkable on its own terms by pointing collect_filename_issues at its root, where its dataset_description.json declares DatasetType: derivative and the derivative rules apply. Measured on 85 real datasets: 513 findings before, 26 after, and the 26 that remain are genuine (a .tsv.bak_ backup file, a datacite.yml, space-CTF on raw anat). A separate check confirmed the schema-sourced NOT_INCLUDED changed only message text: across those datasets no finding's code, location, severity or rule differs from the previous implementation. Tests Two tests pin the derivatives boundary, from both sides: a derivative is not flagged when validating the parent, and is validated correctly when it is the root. The build() helper now creates its own root directory so it can make a nested dataset. 50 tests pass; coverage is 94% on filename_checks and 100% on issues. ruff, mypy and codespell are clean on the new files; the remaining ruff findings are pre-existing ones in context.py, bids_validator.py and types/_typings.py that also fail on a clean checkout of upstream main. --- docs/filename_issues_module.md | 31 +++++++++++ src/bids_validator/__main__.py | 77 +++++++++++++------------- src/bids_validator/filename_checks.py | 78 +++++++++++++++++++++++---- tests/test_filename_checks.py | 31 +++++++++++ 4 files changed, 165 insertions(+), 52 deletions(-) diff --git a/docs/filename_issues_module.md b/docs/filename_issues_module.md index 393694d..98e5925 100644 --- a/docs/filename_issues_module.md +++ b/docs/filename_issues_module.md @@ -33,6 +33,27 @@ Now each problem is a typed `Issue` with a specific code: The finding says which rule was applied and what exactly failed, and it serialises straight to JSON. +## The command line uses it + +`python -m bids_validator ` runs these checks. It previously walked the tree +itself and called `is_bids` on each path, which duplicated the walk and produced the +yes/no output above. It now calls `collect_filename_issues`, so the CLI and a library +caller run exactly the same checks and cannot drift apart. + +```console +$ python -m bids_validator my_dataset +error: MISSING_REQUIRED_ENTITY: sub-01/func/sub-01_bold.nii.gz + missing required entities: task + +1 error(s), 0 warning(s) +$ echo $? +1 +``` + +`-v` additionally prints the schema rule each finding came from. The exit code is `1` +when there are errors and `0` otherwise, so the result is readable from a CI job and +not only from the printed text. + ## Architecture The BIDS schema describes every legal filename: which suffix belongs in which @@ -73,6 +94,15 @@ Default ignores mirror the reference TypeScript validator: `.git**`, `.*`, treated as single units: the recording's own name is validated, but the walk does not descend into it, so its vendor-named internals are never name-checked. +`derivatives/` is a dataset boundary and is not descended into either. A derivative +follows `rules.files.deriv`, not the raw rules of the dataset it sits inside, so +checking it against its parent's rules would report errors for legal files. The +reference validator draws the same boundary, in `src/validators/bids.ts`. To check a +derivative, point `collect_filename_issues` at the derivative's own root: its +`dataset_description.json` declares `DatasetType: derivative`, and the derivative rules +then apply. On a corpus of 85 real datasets this one boundary is the difference between +26 findings and 513. + ### Where the codes come from Schema-defined `rules.checks` carry their own issue code, but the structural @@ -361,6 +391,7 @@ instances. | `build_ignore` | `(tree: FileTree) -> IgnoreMany` | The defaults plus the dataset's `.bidsignore`. | | `filename_issues` | `(context: Context) -> list[Issue]` | All findings for one file. The unit a future rule engine would call. | | `DEFAULT_IGNORES` | `tuple[str, ...]` | Mirrors the reference validator's `defaultIgnores`. | +| `DERIVATIVES_DIR` | `str` | The directory name that ends the walk, because it starts another dataset. | | `FILENAME_ISSUES` | `dict[str, str]` | The ten codes with the reference's reason text. | `filename_issues` takes a `Context` and returns a list rather than mutating a diff --git a/src/bids_validator/__main__.py b/src/bids_validator/__main__.py index befcc7c..f73f86f 100644 --- a/src/bids_validator/__main__.py +++ b/src/bids_validator/__main__.py @@ -8,52 +8,24 @@ raise SystemExit(1) from None import sys -from collections.abc import Iterator from typing import Annotated from bidsschematools.schema import load_schema -from bidsschematools.types.context import Subject from bidsschematools.types.namespace import Namespace -from bids_validator import BIDSValidator -from bids_validator.context import Context, Dataset, Sessions +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.issues import DatasetIssues, Severity from bids_validator.types.files import FileTree app = typer.Typer() -def is_subject_dir(tree: FileTree) -> bool: - return tree.name.startswith('sub-') +def validate(tree: FileTree, schema: Namespace, verbose: bool = False) -> DatasetIssues: + """Check every filename in the dataset against the schema and report what is wrong. - -def walk(tree: FileTree, dataset: Dataset, subject: Subject | None = None) -> Iterator[Context]: - """Iterate over children of a FileTree and check if they are a directory or file. - - If it's a directory then run again recursively, if it's a file file check the file name is - BIDS compliant. - - Parameters - ---------- - tree : FileTree - FileTree object to iterate over - dataset: Dataset - Object containing properties for entire dataset - subject: Subject - object containing subject and session info - - """ - if subject is None and is_subject_dir(tree): - subject = Subject(Sessions(tree)) - - for child in tree.children.values(): - if child.is_dir: - yield from walk(child, dataset, subject) - else: - yield Context(child, dataset, subject) - - -def validate(tree: FileTree, schema: Namespace) -> None: - """Check if the file path is BIDS compliant. + The walk, the rule matching and the findings all come from + :func:`~bids_validator.filename_checks.collect_filename_issues`, so the CLI and + any library caller run exactly the same checks. Parameters ---------- @@ -61,14 +33,32 @@ def validate(tree: FileTree, schema: Namespace) -> None: Full FileTree object to iterate over and check schema : Namespace Schema object to validate dataset against + verbose : bool + Also print the schema rule each finding came from + + Returns + ------- + DatasetIssues + Every finding, so the caller can set an exit code. """ - validator = BIDSValidator() - dataset = Dataset(tree, schema) + issues = collect_filename_issues(tree, schema) - for file in walk(tree, dataset): - if not validator.is_bids(file.path): - print(f'{file.path} is not a valid bids filename') + for issue in issues: + print(f'{issue.severity.value}: {issue.code}: {issue.location}') + if issue.message: + print(f' {issue.message}') + if verbose and issue.rule: + print(f' rule: {issue.rule}') + + errors = len(issues.by_severity(Severity.ERROR)) + warnings = len(issues.by_severity(Severity.WARNING)) + if issues: + print(f'\n{errors} error(s), {warnings} warning(s)') + else: + print('No filename problems found') + + return issues def show_version() -> None: @@ -119,7 +109,12 @@ def main( schema = load_schema(schema_path) - validate(root_path, schema) + issues = validate(root_path, schema, verbose=verbose) + + # A validator is normally run from a script or CI job, so the outcome has to be + # readable from the exit code, not only from the printed text. + if issues.has_errors: + raise typer.Exit(code=1) if __name__ == '__main__': diff --git a/src/bids_validator/filename_checks.py b/src/bids_validator/filename_checks.py index 70a72be..6c8df70 100644 --- a/src/bids_validator/filename_checks.py +++ b/src/bids_validator/filename_checks.py @@ -25,10 +25,11 @@ from collections.abc import Iterator, Mapping from typing import TYPE_CHECKING, Any +from bidsschematools.types.context import Subject from bidsschematools.types.namespace import Namespace from .bidsignore import Ignore, IgnoreMany -from .context import Context, Dataset +from .context import Context, Dataset, Sessions from .issues import DatasetIssues, Issue, Severity if TYPE_CHECKING: @@ -37,6 +38,7 @@ __all__ = [ 'DEFAULT_IGNORES', + 'DERIVATIVES_DIR', 'FILENAME_ISSUES', 'collect_filename_issues', 'filename_issues', @@ -48,14 +50,33 @@ # directories hold files BIDS does not constrain. DEFAULT_IGNORES = ('.git**', '.*', 'sourcedata/', 'code/', 'stimuli/', 'log/') +# A derivative is a separate dataset that happens to live inside another one, and its +# files follow ``rules.files.deriv`` rather than the raw rules of the dataset around +# them. Checking a derivative against its parent's rules reports errors for perfectly +# legal files, so the walk stops at this boundary. The reference validator does the same +# thing twice over: it drops ``derivatives`` from the tree ("Remove derivatives from the +# main fileTree", ``src/validators/bids.ts``) and then skips any remaining context whose +# path contains it while the root dataset is raw. +# +# To check a derivative, point :func:`collect_filename_issues` at the derivative's own +# root. Its ``dataset_description.json`` declares ``DatasetType: derivative``, and the +# derivative filename rules are then the ones that apply. +DERIVATIVES_DIR = 'derivatives' + # Extensions the BIDS inheritance principle allows to sit higher in the tree than the # data they describe, so they are exempt from the datatype-directory requirement. INHERITABLE_EXTENSIONS = frozenset({'.json', '.tsv'}) -# The filename/path codes this module can emit, with the reference validator's -# reason text. Every one is an error; the reference defines no filename warnings. +# The filename/path codes this module can emit. Every one is an error; the reference +# defines no filename warnings. +# +# NOT_INCLUDED is the one code the BIDS schema itself defines, at rules.errors, so it is +# read from there at runtime by _schema_error rather than repeated here. The other nine +# appear nowhere in the schema (verified against every leaf of schema.json); they come +# from the reference validator's catalog in src/issues/list.ts, and are mirrored here so +# the provenance is explicit and the output stays interchangeable. FILENAME_ISSUES: dict[str, str] = { - 'NOT_INCLUDED': 'Files with such naming scheme are not part of BIDS specification.', + 'NOT_INCLUDED': '(defined by the schema at rules.errors.NotIncluded)', 'ENTITY_WITH_NO_LABEL': 'Found an entity with no label.', 'INVALID_ENTITY_LABEL': ("entity label doesn't match format found for files with this suffix"), 'MISSING_REQUIRED_ENTITY': 'Missing required entity for files with this suffix.', @@ -178,12 +199,14 @@ def filename_issues(context: Context) -> list[Issue]: matched = _find_rule_matches(schema, context) if not matched: + # The schema defines this one, so take its code, level and wording from there. + code, severity, message = _schema_error(schema, 'NotIncluded') return [ Issue( - code='NOT_INCLUDED', - severity=Severity.ERROR, + code=code, + severity=severity, location=relpath, - message=f'{context.file.name} does not match any BIDS naming rule', + message=message or f'{context.file.name} does not match any BIDS naming rule', ) ] @@ -207,7 +230,11 @@ def filename_issues(context: Context) -> list[Issue]: def _walk( - tree: FileTree, dataset: Dataset, recordings: set[str], ignore: HasMatch + tree: FileTree, + dataset: Dataset, + recordings: set[str], + ignore: HasMatch, + subject: Subject | None = None, ) -> Iterator[Context]: """Yield one Context per file, depth first, skipping ignored paths. @@ -215,19 +242,32 @@ def _walk( ``.mefd``, OME-Zarr) is one recording, not a folder of files. It is yielded so its own name is validated, but the walk does not descend, so its vendor-named internals are never name-checked. + + A :data:`DERIVATIVES_DIR` directory is a dataset boundary and is not descended into + at all, since the rules on the far side of it are different ones. + + Each context carries the :class:`Subject` of the enclosing ``sub-*`` directory, so + later content checks have it without a second walk. """ + # Entering a sub-* directory establishes the subject every file below it belongs to. + if subject is None and tree.name.startswith('sub-'): + subject = Subject(Sessions(tree)) + for child in tree.children.values(): if ignore.match(child.relative_path): continue if child.is_dir: + if child.name == DERIVATIVES_DIR: + # A separate dataset with separate rules. See DERIVATIVES_DIR. + continue if any(child.name.endswith(ext) for ext in recordings): # A directory recording is one unit: its NAME is checked like a file's, # but its vendor-named internals are not, so yield it without descending. - yield Context(child, dataset, None) + yield Context(child, dataset, subject) continue - yield from _walk(child, dataset, recordings, ignore) + yield from _walk(child, dataset, recordings, ignore, subject) else: - yield Context(child, dataset, None) + yield Context(child, dataset, subject) # --- rule identification -------------------------------------------------- @@ -697,6 +737,22 @@ def _short(schema: Namespace, long_name: str) -> str: return long_name +def _schema_error(schema: Namespace, name: str) -> tuple[str, Severity, str | None]: + """Read a schema-defined error from ``rules.errors``, as (code, severity, message). + + A handful of structural errors are defined by the schema itself, so their code, + level and wording belong to it rather than to us. Falls back to the entry's own name + and ``error`` if the schema does not define it, which keeps an older schema working. + """ + entry = schema['rules'].get('errors', {}).get(name, {}) + code = str(entry.get('code', name)) + level = str(entry.get('level', 'error')) + severity = Severity.WARNING if level == 'warning' else Severity.ERROR + message = entry.get('message') + # Schema messages are multi-line YAML blocks; a finding's message is one line. + return code, severity, ' '.join(str(message).split()) if message else None + + def _directory_recordings(schema: Namespace) -> set[str]: """Extensions of directory-based recordings, e.g. ``.ds``, ``.mefd``. diff --git a/tests/test_filename_checks.py b/tests/test_filename_checks.py index 6c78de4..b902696 100644 --- a/tests/test_filename_checks.py +++ b/tests/test_filename_checks.py @@ -20,6 +20,7 @@ def build(root: pathlib.Path, *relpaths: str) -> pathlib.Path: """Create a minimal dataset containing the given files.""" + root.mkdir(parents=True, exist_ok=True) (root / 'dataset_description.json').write_text( json.dumps({'Name': 'test', 'BIDSVersion': '1.11.1'}) ) @@ -147,6 +148,36 @@ def test_inheritable_metadata_may_sit_above_the_datatype_directory( assert BIDSValidator().is_bids(f'/{relpath}') +def test_derivatives_are_not_checked_against_the_parent_dataset_rules( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A derivative is a separate dataset; its files must not be judged by raw rules.""" + build(tmp_path, VALID) + deriv = tmp_path / 'derivatives' / 'mypipeline' + build(deriv, 'sub-01/anat/sub-01_desc-preproc_T1w.nii.gz', 'logs/run.txt') + assert codes(tmp_path, schema) == {} + + +def test_a_derivative_can_be_validated_as_its_own_dataset( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """Pointing the checks at the derivative root makes the derivative rules apply.""" + deriv = build(tmp_path, VALID) / 'derivatives' / 'mypipeline' + build(deriv, 'sub-01/anat/sub-01_desc-preproc_T1w.nii.gz') + (deriv / 'dataset_description.json').write_text( + json.dumps( + { + 'Name': 'deriv', + 'BIDSVersion': '1.11.1', + 'DatasetType': 'derivative', + 'GeneratedBy': [{'Name': 'mypipeline'}], + } + ) + ) + # desc- is a derivative-only entity, so this name is legal here and nowhere else. + assert codes(deriv, schema) == {} + + def _make_recording(root: pathlib.Path, name: str) -> None: """Create a CTF-style directory recording with a vendor-named internal file.""" recording = root / 'sub-01' / 'meg' / name From 7d06be4429700650215677b636ceffd061758773 Mon Sep 17 00:00:00 2001 From: karellopez Date: Mon, 14 Sep 2026 16:57:31 +0200 Subject: [PATCH 4/4] test(cli): cover the command line, and document how to use it The previous commit moved the CLI onto the filename issues module but added no tests for it, and the CLI had none to begin with. This adds them and writes up the usage. Tests tests/test_main.py covers what is genuinely the CLI's own behaviour: what it prints for a dataset with problems, what it prints for a clean one, that -v appends the schema rule, and that the exit code is 1 with errors and 0 without. The sixth test is the point of the file. It builds a dataset with several different problems, collects the findings from collect_filename_issues directly, runs the CLI, parses the printed lines back into codes and locations, and asserts the two sets are identical. The CLI used to carry its own tree walk and call is_bids per path, which could drift from the module's results; this test goes red if that ever comes back. The dataset helper is imported from tests/test_filename_checks.py rather than copied, so there is one way to build a fixture dataset in the suite. Coverage of __main__.py goes from 0% to 81%, with filename_checks at 94% and issues at 100%. 56 tests pass. Documentation docs/filename_issues_module.md gains a full command line section: the help output, checking a dataset, the output format, -v for rule provenance, an exit code table, shell and GitHub Actions snippets, checking a derivative on its own terms, --schema-path, and the equivalent Python for callers who want the findings as objects rather than text. Every example is captured from a real run. One fix while writing it: the --schema-path example said schema.json, but load_schema takes the schema's YAML directory or a single YAML file, not a built schema.json. Also removes a stale tests/validation/__pycache__ left behind by the validation package that was moved to its own branch. --- docs/filename_issues_module.md | 141 +++++++++++++++++++++++++++++++-- tests/test_main.py | 121 ++++++++++++++++++++++++++++ 2 files changed, 256 insertions(+), 6 deletions(-) create mode 100644 tests/test_main.py diff --git a/docs/filename_issues_module.md b/docs/filename_issues_module.md index 98e5925..0a1c460 100644 --- a/docs/filename_issues_module.md +++ b/docs/filename_issues_module.md @@ -33,26 +33,155 @@ Now each problem is a typed `Issue` with a specific code: The finding says which rule was applied and what exactly failed, and it serialises straight to JSON. -## The command line uses it +## Using the command line `python -m bids_validator ` runs these checks. It previously walked the tree itself and called `is_bids` on each path, which duplicated the walk and produced the yes/no output above. It now calls `collect_filename_issues`, so the CLI and a library -caller run exactly the same checks and cannot drift apart. +caller run the same checks by construction and cannot drift apart. + +``` +Usage: python -m bids_validator [OPTIONS] {bids_path} + +Arguments: + bids_path [required] + +Options: + --schema-path Validate against a schema other than the bundled one + --verbose -v Also print the schema rule behind each finding + --version Show the version and exit + --help Show this message and exit +``` + +### Checking a dataset ```console $ python -m bids_validator my_dataset +error: NOT_INCLUDED: sub-01/notes.txt + Files with such naming scheme are not part of BIDS specification. This error is most commonly caused by typos in filenames that make them not BIDS compatible. Please consult the specification and make sure your files are named correctly. +error: INVALID_ENTITY_LABEL: sub-01/anat/sub-01_acq-a!b_T1w.nii.gz + label 'a!b' for entity 'acq' does not match /[0-9a-zA-Z+]+/ +error: MISSING_REQUIRED_ENTITY: sub-01/func/sub-01_bold.nii.gz + missing required entities: task + +3 error(s), 0 warning(s) +``` + +Each finding is three fields on one line, then the detail indented beneath: + +``` +severity: CODE: location + message +``` + +A clean dataset says so explicitly rather than printing nothing: + +```console +$ python -m bids_validator my_clean_dataset +No filename problems found +``` + +### Seeing which rule fired + +`-v` appends the schema path the finding came from, so a reader can go and check the +standard instead of taking the message on trust. + +```console +$ python -m bids_validator my_dataset -v +... error: MISSING_REQUIRED_ENTITY: sub-01/func/sub-01_bold.nii.gz missing required entities: task + rule: rules.files.raw.func.func -1 error(s), 0 warning(s) +3 error(s), 0 warning(s) +``` + +### Exit codes + +| Code | Meaning | +|---|---| +| `0` | No errors. Warnings may still have been printed. | +| `1` | At least one error. | + +So the result is readable from a script or CI job and not only from the printed text: + +```console +$ python -m bids_validator my_dataset $ echo $? 1 ``` -`-v` additionally prints the schema rule each finding came from. The exit code is `1` -when there are errors and `0` otherwise, so the result is readable from a CI job and -not only from the printed text. +```bash +# fail a build when the dataset is not valid +python -m bids_validator "$DATASET" || exit 1 + +# or branch on it +if python -m bids_validator "$DATASET" > report.txt; then + echo "filenames are fine" +else + echo "problems found:"; cat report.txt +fi +``` + +```yaml +# in GitHub Actions, a non-zero exit fails the step automatically +- name: Validate BIDS filenames + run: python -m bids_validator ./my_dataset +``` + +### Checking a derivative + +`derivatives/` is skipped when checking the dataset around it, because a derivative +follows different rules (see the section above). Point the CLI at the derivative's own +root to check it on its own terms: + +```console +$ python -m bids_validator my_dataset/derivatives/mypipeline +No filename problems found +``` + +### Validating against a different schema + +By default the schema bundled with `bidsschematools` is used. `--schema-path` accepts +another one, which is how you check a dataset against a newer or older version of the +standard. It takes the schema's YAML directory (or a single YAML file), not a built +`schema.json`: + +```console +$ python -m bids_validator my_dataset --schema-path ~/bids-specification/src/schema +``` + +Because every check reads the schema rather than hardcoding BIDS knowledge, pointing at +a different schema is all that is needed to validate against a different version of the +standard. + +### Doing the same thing from Python + +The CLI is a thin wrapper, so anything it does is available directly, with the findings +as objects rather than text: + +```python +from bidsschematools.schema import load_schema + +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +issues = collect_filename_issues( + FileTree.read_from_filesystem('my_dataset'), load_schema() +) + +for issue in issues: + print(issue.code, issue.location, issue.severity, issue.rule) + +errors = issues.by_severity(Severity.ERROR) +if issues.has_errors: + raise SystemExit(f'{len(errors)} filename error(s)') +``` + +`docs/example_filename_issues.py` is a runnable version of this. With no arguments it +generates a dataset containing one example of every issue code; given a path, it +validates that dataset instead. ## Architecture diff --git a/tests/test_main.py b/tests/test_main.py new file mode 100644 index 0000000..50c398d --- /dev/null +++ b/tests/test_main.py @@ -0,0 +1,121 @@ +"""Tests for the command line. + +The CLI is a thin front end over :mod:`bids_validator.filename_checks`. These tests +cover the part that is genuinely the CLI's own: what it prints, and what it exits with. +The last test is the important one. It pins the CLI to the module, so the duplicated +walk that used to live in ``__main__`` cannot creep back in. +""" + +import pathlib + +import pytest +from bidsschematools.types.namespace import Namespace +from typer.testing import CliRunner + +from bids_validator.__main__ import app, validate +from bids_validator.filename_checks import collect_filename_issues +from bids_validator.issues import Severity +from bids_validator.types.files import FileTree + +from .test_filename_checks import VALID, build + +runner = CliRunner() + +BROKEN = 'sub-01/func/sub-01_bold.nii.gz' # missing the required task entity + + +def test_validate_returns_the_findings_and_prints_them( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + """Every finding is both returned to the caller and shown to the user.""" + build(tmp_path, BROKEN) + issues = validate(FileTree.read_from_filesystem(str(tmp_path)), schema) + + assert len(issues) == 1 + assert issues.has_errors + + out = capsys.readouterr().out + assert 'error: MISSING_REQUIRED_ENTITY: sub-01/func/sub-01_bold.nii.gz' in out + assert 'missing required entities: task' in out + assert '1 error(s), 0 warning(s)' in out + + +def test_validate_says_so_when_there_is_nothing_wrong( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + """A clean dataset gets a clear answer, not silence.""" + build(tmp_path, VALID) + issues = validate(FileTree.read_from_filesystem(str(tmp_path)), schema) + + assert len(issues) == 0 + assert not issues.has_errors + assert 'No filename problems found' in capsys.readouterr().out + + +def test_verbose_adds_the_schema_rule( + tmp_path: pathlib.Path, schema: Namespace, capsys: pytest.CaptureFixture[str] +) -> None: + """``-v`` shows which rule produced the finding, so a user can check the standard.""" + build(tmp_path, BROKEN) + tree = FileTree.read_from_filesystem(str(tmp_path)) + + validate(tree, schema, verbose=False) + assert 'rule:' not in capsys.readouterr().out + + validate(tree, schema, verbose=True) + assert 'rule: rules.files.raw.func.func' in capsys.readouterr().out + + +def test_cli_exit_code_is_one_when_there_are_errors( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """A CI job reads the exit code, not the printed text.""" + build(tmp_path, BROKEN) + result = runner.invoke(app, [str(tmp_path)]) + assert result.exit_code == 1 + assert 'MISSING_REQUIRED_ENTITY' in result.stdout + + +def test_cli_exit_code_is_zero_on_a_clean_dataset( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """The converse: a good dataset must not fail the build.""" + build(tmp_path, VALID) + result = runner.invoke(app, [str(tmp_path)]) + assert result.exit_code == 0 + assert 'No filename problems found' in result.stdout + + +def test_cli_reports_exactly_what_the_module_reports( + tmp_path: pathlib.Path, schema: Namespace +) -> None: + """The CLI must not grow a second implementation of the checks. + + It once had its own tree walk and called ``is_bids`` per path, which could drift + from the module's results. Now it delegates, and this test fails if anything is + ever reported by one and not the other. + """ + build( + tmp_path, + VALID, + BROKEN, + 'sub-01/notes.txt', + 'sub-01/anat/sub-01_T1w.txt', + 'sub-02/anat/sub-01_T1w.nii.gz', + ) + expected = { + (i.code, i.location) + for i in collect_filename_issues(FileTree.read_from_filesystem(str(tmp_path)), schema) + } + assert expected, 'the fixture should produce findings, otherwise this proves nothing' + + result = runner.invoke(app, [str(tmp_path)]) + + reported = set() + for line in result.stdout.splitlines(): + # "severity: CODE: location" + parts = line.split(': ') + if len(parts) == 3 and parts[0] in {s.value for s in Severity}: + reported.add((parts[1], parts[2])) + + assert reported == expected