Repository navigation
Fix #981: make generic_file.ext optional in generic_file schema - #982
Closed
claude[bot] wants to merge 1 commit into
Closed
claude[bot] wants to merge 1 commit into
claude[bot] wants to merge 1 commit into
Conversation
The generic_file class is used in two legitimate modes: describe-only (the default in ndi.fun.doc.gene.makeSourceFile, where the doc records the source file's name and MD5 checksum without ingesting a copy) and attach (attachFile=true, where the file itself is bound to the doc). The schema declared "generic_file.ext" with mustbenotempty: 1, which was tolerated by an earlier DID-matlab validator that fell through to isvalid=1 when nothing was bound. DID's issue #199 tightening now refuses a required file-list entry that has no matching file_info binding, so every session.database_add on a describe-only doc fails. Loosen the schema to mustbenotempty: 0. Both modes still validate: the file_list still declares generic_file.ext (so Step 1 of did.database.checkfiles passes), the attach path continues to bind the file, and the describe path is no longer refused. This matches the pattern used by other optional-file schemas such as pyraview. Fixes #981. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Cn7RipE1szEPizasFa1EEo
Contributor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #982 +/- ##
=======================================
Coverage 29.19% 29.19%
=======================================
Files 853 853
Lines 43037 43037
=======================================
Hits 12566 12566
Misses 30471 30471 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
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.
Summary
Fixes #981. The 12
TestGeneFromFilesfailures onmain— every one of the form:— come from
session.database_add(...)on ageneric_filedoc produced byndi.fun.doc.gene.makeSourceFileunder its defaultattachFile=false. DID-matlab's issue-#199 tightening (did.database.checkfilesatdatabase.m:1710) now refuses this case that used to fall through toisvalid = 1.The
generic_fileclass is used in two legitimate modes:.gefcase, documented onmakeSourceFileitself.attachFile=true): the file is bound to the doc.This PR flips
mustbenotempty: 1→mustbenotempty: 0on thegeneric_file.extfile entry ingeneric_file_schema.json. Both modes validate cleanly under DID's tightened check:setdiff(expectedNames, actual_file_list)) still passes because the definition file continues to declaregeneric_file.extinfile_list.generic_file.ext; the attach path still binds it, the describe path is no longer refused.This matches the pattern already used by
pyraview_schema.jsonand the tile files inspatialGeneExpressionTiles_schema.json, which declare optional bindings withmustbenotempty: 0.Options considered:
makeSourceFile— would satisfy the schema but would forcetestSourceFileDescribesWithoutIngestingAPayloadto stop assertingverifyEmpty(doc.current_file_list()), which is exactly the documented invariant of the describe-only default.The check being loosened only fired at write time on a doc whose
file_listdeclaredgeneric_file.extbut bound nothing. SincemakeSourceFileis the only producer ofgeneric_filedocs in the codebase and its two branches areadd_file(...)or nothing, there is no attach-path bug the required-file check was silently catching.Expected effect on CI
All 12 previously-failing
TestGeneFromFilestests should pass:testPyramidTilesCarryTheSourceFileDependencytestCellsCarryTheSourceFileDependencytestFromGefBuildsTheWholeLaddertestFromGefTakesTheChipAndPixelSizeFromTheFiletestFromGefRecordsTheSourceFileByDefaulttestFromGefReportsProgressThroughTheWholeRuntestFromGefWithoutAProgressFcnIsSilentRatherThanBrokentestFromGefRefusesWithoutASubject(was reporting'DID:Database:ValidationFiles'beforefromGEFreached the subject-required check)testFromCellBinBuildsCellsAgainstThePyramidtestFromCellBinRecordsWhatTheReaderConcludedNotWhatWasAskedtestFromCellBinIngestsARequestedLabelingtestFromCellBinTakesUnsupervisedFromTheCallerNotTheFileThe
attachFile=truetest (testSourceFileCanAttachWhenAsked) also continues to pass — the schema now permits the file binding rather than requiring it, and the code still binds when asked.Once this lands, PR #979 will go green on rebase (its diff is orthogonal — lightsheet scaffold).
Test plan
Run NDI Test Suite No Cloud— expect the 12TestGeneFromFilesfailures to clear.ndi.unittest.fun.doc.gene.TestGeneFromFilesand confirm all pass.generic_filedocs beyondmakeSourceFile; the schema change does not affect docs that already bind a file.🤖 Generated with Claude Code
https://claude.ai/code/session_01Cn7RipE1szEPizasFa1EEo
Generated by Claude Code