PR-17: migrate time core to v2 (#454)#517
PR-17: migrate time core to v2 (#454)#517charliecreates[bot] wants to merge 1 commit intocharlie/issue-454-v2-pr17-migrate-ids-names-v2from
Conversation
|
orrery preview: https://pr-517.orrery-c4f.pages.dev/ |
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
The v2 migration looks mechanically consistent, but contract.result is currently an empty object schema everywhere, which is likely incorrect for many of these methods and can undermine validation/comparison in the v2 harness. Comparison defaults are applied inconsistently (et2utc missing) and inappropriately for methods that return strings (e.g., spiceVersion, tkvrsn, timout), which can mask real mismatches or create misleading specs.
Summary of changes
Summary
Migrates parity-checking time method specs from @v1 to @v2 schema across multiple methods.
What changed
- Replaced legacy v1 spec format (
id/kind/contractMethod/.../uses/cases) with v2 structure:schemaVersion: 2manifest(id,kind)contract(contractMethod,canonicalMethod,aliases,result)setup.kernels: [$FIXTURES/basic-time]workflow.steps: [{ op: invokeLegacyCall }]- Optional
defaults.comparetolerances (tolAbs,tolRel) added to most specs
- Files affected:
- Deleted and re-added as v2:
deltet,spiceVersion,str2et,timdef,timout,tkvrsn,tparse,tpictr,unitim - Renamed
et2utc@v1.yml→et2utc@v2.ymland updated content to v2 schema
- Deleted and re-added as v2:
Notable behavioral/config changes
- Introduced global default numeric comparison tolerances (
1e-12) for most methods. et2utc@v2.ymldoes not includedefaults.comparewhile others do.
| contract: | ||
| contractMethod: time.deltet | ||
| canonicalMethod: time.deltet | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} |
There was a problem hiding this comment.
contract.result is defined as an empty object schema (type: object with empty properties). For several of these methods (e.g., time.deltet, time.unitim, time.str2et, time.timout), the return is typically a scalar/primitive (number or string) rather than an object. If the v2 runner uses contract.result to drive decoding/validation/comparison, this can cause false positives/negatives or skip meaningful result assertions.
Suggestion
Define contract.result to match the actual return shape for the method (e.g., type: number/type: string/tuple-like array/object with explicit properties), or omit result if the v2 schema supports that and the harness infers it. Example for numeric return:
contract:
contractMethod: time.deltet
canonicalMethod: time.deltet
aliases: []
result:
type: numberReply with "@CharlieHelps yes please" if you'd like me to add a commit updating the result schemas across these time specs consistently.
| schemaVersion: 2 | ||
| manifest: | ||
| id: methods/time/et2utc@v2 | ||
| kind: method | ||
| contract: | ||
| contractMethod: time.et2utc | ||
| canonicalMethod: time.et2utc | ||
| aliases: [] | ||
| result: | ||
| type: object | ||
| properties: {} | ||
| setup: | ||
| kernels: | ||
| - $FIXTURES/basic-time | ||
| workflow: | ||
| steps: | ||
| - op: invokeLegacyCall |
There was a problem hiding this comment.
Most v2 specs add defaults.compare.tolAbs/tolRel, but this one doesn’t. That inconsistency can lead to different comparison behavior across methods and flaky parity checks when floating-point outputs are involved (even if this particular case currently only asserts ok: false).
Suggestion
Either add the same defaults.compare block here for consistency, or explicitly document/encode why et2utc is intentionally different (e.g., it returns strings and should be compared exactly). If et2utc returns a string, consider setting a string-appropriate compare strategy instead of numeric tolerances. Reply with "@CharlieHelps yes please" if you'd like me to add a commit to align et2utc@v2.yml with the other v2 time specs.
| defaults: | ||
| compare: | ||
| tolAbs: 1e-12 | ||
| tolRel: 1e-12 |
There was a problem hiding this comment.
Applying numeric tolerances (tolAbs/tolRel) globally to a method that returns a version string risks masking real mismatches if the compare layer mistakenly treats outputs numerically/coercively or applies tolerance logic broadly. Even if the harness is smart, this is an easy footgun for future changes.
Suggestion
Remove defaults.compare for methods that return strings (like time.spiceVersion), or switch to an explicit string comparison mode if supported by the spec schema (e.g., exact match). Reply with "@CharlieHelps yes please" if you'd like me to add a commit removing/adjusting tolerances for string-returning time methods.
| defaults: | ||
| compare: | ||
| tolAbs: 1e-12 | ||
| tolRel: 1e-12 |
There was a problem hiding this comment.
The same concern as above: defaults.compare numeric tolerances don’t make sense for time.timout, which is expected to return a formatted time string. Keeping tolerances here can hide comparison configuration errors and makes intent unclear.
Suggestion
Drop defaults.compare.tolAbs/tolRel for time.timout@v2 (string result) or replace it with a string/exact compare configuration if the framework supports it. Reply with "@CharlieHelps yes please" if you'd like me to add a commit making this change.
| defaults: | ||
| compare: | ||
| tolAbs: 1e-12 | ||
| tolRel: 1e-12 |
There was a problem hiding this comment.
defaults.compare numeric tolerances on time.tkvrsn (version string) has the same mismatch risk and makes the spec misleading about what is being compared.
Suggestion
Remove numeric tolerances for time.tkvrsn@v2 or configure string comparison explicitly. Reply with "@CharlieHelps yes please" if you'd like me to add a commit adjusting this spec.
1 participant
Stack step PR-17 in the issue #454 migration chain.