Not a fault in the script, which is a stub and says so. The problem is that the pull request template treats it as a gate.
What the template asks
- [ ] Validation scripts pass (python run_all_checks.py)
What the script does
data_validation/run_all_checks.py carries a # TODO: Import and run each validator, prints "Validators not yet implemented - see validators/ for contribution guide", and returns without setting a non-zero exit code.
python data_validation/run_all_checks.py --dataset datasets/
echo $?
Provided the --dataset path exists, the exit status is 0, and any other existing directory gives the same result. The only way to get a non-zero status is to pass a path that does not exist, which fails the directory check rather than any validation. So there is no state of the repository in which this reports a failure.
It is also not wired into any workflow. The four in .github/workflows are dependency-review, pages, scanner-lint and scanner-selftest. That makes the box a human assertion rather than a job result, which I think is the worse of the two, because the contributor is the one signing it.
Why it is worth an issue anyway
A contributor runs it in good faith, sees output and no error, and ticks the box. The merged pull request then carries a record saying a validation passed when nothing was validated. Nothing downstream consumes that today, which is why this is small, but it is the kind of small that gets wired into CI later by someone who assumes the script means what the checklist says it means.
I hit this running the script, and going through the open issues and pull requests afterwards I saw #59 raises the same thing. The box is left unticked there because the script "exits without running a check, so a pass from it would not mean anything", with an ask about whether there is another gate to run instead. Worth its own item, because a remark inside one contributor's pull request gets read once and then merges out of sight, and because the consequence is not confined to that pull request. The wording asks every contributor to certify this, and a careful one leaves the box empty and looks incomplete beside a careless one who ticks it. As it stands the checklist grades honesty below inattention.
Related, and I think the same defect from the other side. #60 reports that scripts/generate.js stamps the current date into committed bundles, so a regenerate-and-compare check fails on a clean checkout regardless of input. That one always fails and will end up ignored or switched off; this one always passes and gets ticked. Neither is measuring anything, and they are cheaper to fix together than separately.
Three ways out, cheapest first
-
Change the template. Drop the line, or reword it so the box is knowingly not applicable while the validators are unimplemented. One edit, fixes the false record and nothing else.
-
Make the script exit non-zero while it is unimplemented. Then the tick cannot be obtained honestly by anyone, including contributors who never read this issue. This is the one that does not rot, because it fails closed rather than relying on everyone remembering.
-
Implement the validators. The real answer, the most work, and it does not need to block either of the others.
I would do 2 now and 3 when there is time. 1 on its own leaves a live script that reports success, and sooner or later somebody wires it into CI.
Happy to send the one-line change for 2 if that is the direction you want.
Not a fault in the script, which is a stub and says so. The problem is that the pull request template treats it as a gate.
What the template asks
- [ ] Validation scripts pass (python run_all_checks.py)What the script does
data_validation/run_all_checks.pycarries a# TODO: Import and run each validator, prints "Validators not yet implemented - see validators/ for contribution guide", and returns without setting a non-zero exit code.Provided the
--datasetpath exists, the exit status is0, and any other existing directory gives the same result. The only way to get a non-zero status is to pass a path that does not exist, which fails the directory check rather than any validation. So there is no state of the repository in which this reports a failure.It is also not wired into any workflow. The four in
.github/workflowsare dependency-review, pages, scanner-lint and scanner-selftest. That makes the box a human assertion rather than a job result, which I think is the worse of the two, because the contributor is the one signing it.Why it is worth an issue anyway
A contributor runs it in good faith, sees output and no error, and ticks the box. The merged pull request then carries a record saying a validation passed when nothing was validated. Nothing downstream consumes that today, which is why this is small, but it is the kind of small that gets wired into CI later by someone who assumes the script means what the checklist says it means.
I hit this running the script, and going through the open issues and pull requests afterwards I saw #59 raises the same thing. The box is left unticked there because the script "exits without running a check, so a pass from it would not mean anything", with an ask about whether there is another gate to run instead. Worth its own item, because a remark inside one contributor's pull request gets read once and then merges out of sight, and because the consequence is not confined to that pull request. The wording asks every contributor to certify this, and a careful one leaves the box empty and looks incomplete beside a careless one who ticks it. As it stands the checklist grades honesty below inattention.
Related, and I think the same defect from the other side. #60 reports that
scripts/generate.jsstamps the current date into committed bundles, so a regenerate-and-compare check fails on a clean checkout regardless of input. That one always fails and will end up ignored or switched off; this one always passes and gets ticked. Neither is measuring anything, and they are cheaper to fix together than separately.Three ways out, cheapest first
Change the template. Drop the line, or reword it so the box is knowingly not applicable while the validators are unimplemented. One edit, fixes the false record and nothing else.
Make the script exit non-zero while it is unimplemented. Then the tick cannot be obtained honestly by anyone, including contributors who never read this issue. This is the one that does not rot, because it fails closed rather than relying on everyone remembering.
Implement the validators. The real answer, the most work, and it does not need to block either of the others.
I would do 2 now and 3 when there is time. 1 on its own leaves a live script that reports success, and sooner or later somebody wires it into CI.
Happy to send the one-line change for 2 if that is the direction you want.