Clarify 0087 directive-order harness comments#226
Merged
Conversation
Address CoPilot review on #225 (both threads resolved there but the edits landed after the squash-merge). - test_observability.py: reword the 135 support-comment from "key order" to "document (dict insertion) order", matching the loader guard-comment and removing the sorted-by-key ambiguity. - test_fixture_parsing.py: correct the 135 parse-deferral reason. It read "not implemented", but 0087 is implemented -- only the parse shape is deferred, because 135 reuses the augment/capture directive shapes the cross-cap parser does not model (as siblings 043-046), runtime-driven in test_observability. Comment-only; no behavior change.
There was a problem hiding this comment.
Pull request overview
This PR updates conformance-test comments to more precisely describe Proposal 0087’s within-node directive execution order and why fixture 135 is parse-deferred at the cross-capability parser layer (while still being runtime-covered).
Changes:
- Reword fixture 135’s harness comment to clarify it relies on fixture document (dict insertion) order rather than a sorted key ordering interpretation.
- Correct fixture 135’s parse-deferral rationale to reflect that 0087 is implemented, but the cross-cap parser does not model the reused directive shapes.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/conformance/test_observability.py | Clarifies the fixture 135 harness comment around directive execution ordering. |
| tests/conformance/test_fixture_parsing.py | Updates the deferred-parse reason for fixture 135 to match current implementation reality. |
Address CoPilot review on #226: the _SUPPORTED_FIXTURES comment points readers at _apply_metadata_directives for the document-order behavior, but that function's own comment still said "IN KEY ORDER" / "YAML key order" -- the sorted-by-key ambiguity this branch removes at the call site. Reword it to "document order" so both spots agree. Comment-only; no behavior change.
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.
Follow-up to #225. Both CoPilot review threads on #225 were resolved
with these edits, but they landed after the squash-merge, so main
didn't carry them yet.
test_observability.py: reword the fixture 135 support-comment from"key order" to "document (dict insertion) order", matching the loader
guard-comment and removing the sorted-by-key ambiguity.
test_fixture_parsing.py: correct the 135 parse-deferral reason. Itread "not implemented", which is misleading — 0087 is implemented;
only the parse shape is deferred, because 135 reuses the
augment_metadata / capture_invocation_metadata_into directive shapes
the cross-capability parser deliberately does not model (the same
reason its siblings 043-046 are deferred at that layer). Runtime
coverage is via test_observability.
Comment-only, no behavior change. Full suite green.