feat: add issues module with an early implementation for the filename validator - #86
Open
karellopez wants to merge 4 commits into
Open
karellopez wants to merge 4 commits into
karellopez wants to merge 4 commits into
Conversation
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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #86 +/- ##
==========================================
+ Coverage 90.56% 92.32% +1.75%
==========================================
Files 13 21 +8
Lines 890 1654 +764
Branches 130 290 +160
==========================================
+ Hits 806 1527 +721
- Misses 50 69 +19
- Partials 34 58 +24 🚀 New features to boost your workflow:
|
…t 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.
…odule 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.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #85
What this adds
Two modules that turn filename validation into structured findings.
bids_validator/issues.pybids_validator/filename_checks.pyValidation currently answers a yes/no question.
BIDSValidator.is_bids()returns aboolean and the CLI prints one line per bad file, which carries no severity, no stable
code, and no structure a program can consume.
context.pyalready anticipates this:ValidationErroris a stub whose body is"""TODO: Add issue structure.""".A problem is now a typed record:
Scope is names and paths only. Nothing here opens a file or reads its contents.
BIDSValidator.is_bids()is untouched, since pybids and mne-bids depend on it.This PR adds 6 files and modifies none.
Design notes
attrs, not pydantic or msgspec, matchingcontext.pyandtypes/files.py.issues.pyimports nothing from the package and cannever take part in an import cycle. Any future check module can depend on it.
filename_checks.pyreadsrules.filesfrom the schema and identifies which rule(s) apath matches, then reports one specific code per kind of failure. Nothing about BIDS is
hardcoded; only the ten issue code strings are constants, because the schema does not
define them. They come from the TypeScript validator's catalog (
src/issues/list.ts),mirrored in
FILENAME_ISSUESso the provenance is explicit and the output staysinterchangeable.
.git**,.*,sourcedata/,code/,stimuli/,log/) in addition to.bidsignore. Without them,dotfiles such as
.DS_Storeare reported, which the TypeScript validator never does..ds, MEF.mefd, OME-Zarr) are treated as single units: thewalk does not descend into them.
than once per file (measured: 0.8 ms first call, 0.0005 ms thereafter, 179 rules). The
walk is a generator, so a large dataset holds one context at a time.
Field naming and JSON output
Following your note in #85: the model uses snake_case internally, and reporting output
should follow the TypeScript validator's camelCase interface. Four of the six fields
already match; the mapping for a future reporter is:
codecodeseverityseveritylocationlocationrulerulesub_codesubCodemessageissueMessageNo JSON reporter is included in this PR, so nothing emits either form yet.
attrs.asdict(issue)produces the internal names and is only a convenience forinspection, not a reporting format. I will add the camelCase mapping when the reporter
lands, or I can add a small
to_reporting_dict()here if you would rather the conventionwere in place from the start.
One deliberate difference from the TypeScript validator
A data file outside any recognised datatype directory is reported as
INVALID_LOCATION:The TypeScript validator misses this.
findDatatypereturns an empty string when theparent folder is not a known datatype (
src/schema/datatypes.ts), andDATATYPE_MISMATCHis gated on that value being truthy (
src/validators/filenameValidate.ts). Meanwhilesuffix-based rule matching ignores the datatype entirely
(
src/validators/filenameIdentify.ts), so the file still matches a rule and is notNOT_INCLUDEDeither.The legacy
is_bidsregexes do catch it, because they cover the whole path. Sincethis module is the schema-driven successor to that check, dropping the case would lose
coverage users already have:
is_bids/sub-01/anat/sub-01_T1w.nii.gz/sub-01/foo/sub-01_T1w.nii.gzINVALID_LOCATION/sub-01/sub-01_T1w.nii.gzINVALID_LOCATION/sub-01/func/sub-01_T1w.nii.gzDATATYPE_MISMATCHDATATYPE_MISMATCHMetadata files are exempt: the inheritance principle lets a
.jsonor.tsvsit higherin the tree than the data it describes, so
sub-01/sub-01_T1w.jsonandtask-rest_bold.jsonat the dataset root are correct and are not flagged.Happy to put this behind a flag if you would rather keep strict parity with the
TypeScript validator by default. I think it is also worth reporting upstream to
bids-standard/bids-validatoras a genuine miss.Tests
Following the existing conventions: plain pytest, the session-scoped
schemafixturefrom
tests/conftest.py,@pytest.mark.parametrize, no new fixture machinery and no newdependency. No datalad usage added (per #10).
tests/test_issues.pycovers the model: defaults, both severities, the collectionhelpers, and an
attrs.asdictround trip.tests/test_filename_checks.pyis table driven with one case per issue code, plusthe default ignores,
.bidsignore, therulefield, catalog completeness, and thedatatype-directory cases cross-checked against
is_bidsto pin the intent.28 tests, all passing.
CI status
build-test-deploypasses on the full Python 3.10 to 3.14 matrix:The
stylejob fails, for reasons that predate this PR.ruff check src/reports 8findings, none of them in the files this PR adds:
src/bids_validator/context.pysrc/bids_validator/bids_validator.pysrc/bids_validator/types/_typings.pysrc/bids_validator/issues.py(added here)src/bids_validator/filename_checks.py(added here)Rules:
PIE790x2,PLW0120,PYI036,RUF022,SIM102,SIM118,TRY004.I verified this is not caused by the PR by checking out
mainat 5a361c4 in a cleanworktree, where neither added module exists, and running the same command. It produces
the identical
Found 8 errorswith the same rules in the same three files. The cause isthat
tox.inideclaresdeps = ruffunpinned, so CI installs the newest ruff (0.16.2 inthis run) and recently added rules flag existing code, the same situation as the earlier
chore: Resolve ruff complaintscommit.For the two modules and two test files added here,
ruff check,ruff format --diff,codespellandmypyare all clean.I have deliberately not fixed those 8 findings in this PR, to keep the diff to the
feature. Happy to send them as a separate
chorePR if useful. One caveat if I do:TRY004asks to change aValueErrorto aTypeErrorincontext.py, which is abehaviour change, so I would rather you decide that one than have me slip it into a
cleanup.
Coverage
Measured locally on the added modules with the same invocation CI uses:
93% on the added code, above the 80% project and patch targets in
codecov.yml.Docs
docs/filename_issues_module.md: what the modules add, architecture with flowcharts,the ten codes, usage, 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 adataset containing one deliberately broken file per issue code; with a path it
validates your own dataset.
Not included, on purpose
same
Issuetype into the sameDatasetIssues, so they extend this without changingthe output contract.
(per Add a structured issues model and emit findings from the filename validator #85), this can simply replace the current printed output rather than sit behind a
compatibility flag. Happy to do it in this PR or the next, whichever you prefer.
Issuefields (rule provenance, fixes,ignoreseverity, line spans). Left out to keep this PR reviewable, and easy to expand as
necessary per Add a structured issues model and emit findings from the filename validator #85.