Repository navigation
fix: bind ORM path values by field name - #500
Merged
jacebrowning merged 1 commit intoOct 17, 2026
Merged
Conversation
[verified] Independent automated diff review completed.
jacebrowning
self-requested a review
October 17, 2026 15:33
jacebrowning
approved these changes
Oct 17, 2026
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
all()andfilter(), intervening non-path fields, recursive paths, exclusions, and alternate patterns with defaults.Problem
Manager.all()discards the names returned byparse()and passes path values positionally toget(). When declaration order differs from placeholder order, values bind to the wrong fields and loading raisesFileNotFoundError. Non-path fields before path fields have the same problem.Solution
Retain the existing path-value normalization and exclusion checks, then associate each value with its parsed field name and call
get()with keyword arguments. This follows the approach invited in #334. Public positionalget()behavior is unchanged;filter()shares the correctedall()path. No type-conversion change for #147 is included.Testing
Executed in a credential-free, network-isolated Linux sandbox with CPython 3.11.15 and lock-pinned test/quality dependencies:
python -m pytest tests/test_orm_usage.py datafiles/tests/test_manager.py -q -p no:cacheprovider --basetemp=/tmp/pytest: 27 passed, 1 known xfail. Restoring the originalmanager.pywith the final tests gives 5 failed, 22 passed, 1 xfail; restoring the fix passes again.python -m pytest --count=2 --random --exitfirst --cov-report=xml -q -p no:cacheprovider --basetemp=/tmp/pytest: baseline 604 passed; fixed 616 passed. Both have 2 macOS-only skips and 10 existing xfails. Each run reports onecache_dirconfiguration warning because the cache plugin is disabled for isolation.python -m black --check datafiles tests,python -m isort --check-only datafiles tests,python -m mypy datafiles tests --config-file=.mypy.ini(45 files),python -m pylint datafiles tests --rcfile=.pylint.ini, andpython -m pydocstyle datafiles tests: pass.python -m mkdocs build --clean --strict: pass.papermill.execute_notebookin both baseline and fixed disposable checkouts: pass. Kernel TCP warnings are present inside the private network namespace.poetry.core.masonry.apibackend: pass; packaged runtime bytes verified.git diff --check, added-line security scan, and independent automated diff review: pass.Other Python versions and native Windows/macOS were not run locally; hosted CI still needs to validate its matrix. No CI approval controls were bypassed.
Related Issue
Fixes #334
Automation disclosure
Implemented, tested, and submitted by Hermes DEV (
hermes:dev) using OpenAI gpt-6-astra, with a separate automated reviewer. No human pre-review or manual validation is claimed.