Skip to content

Strip Ms suffix for duration properties#1339

Merged
stephentoub merged 2 commits into
mainfrom
stephentoub/duration-schema-handling
May 19, 2026
Merged

Strip Ms suffix for duration properties#1339
stephentoub merged 2 commits into
mainfrom
stephentoub/duration-schema-handling

Conversation

@stephentoub
Copy link
Copy Markdown
Collaborator

Duration-formatted schema fields already map to timespan-like SDK types in Python and C#, but generated member names still exposed millisecond-oriented wire suffixes like durationMs. This keeps the wire contract unchanged while making the generated SDK surface more idiomatic for duration values.

Summary

  • Strip eligible trailing Ms from duration property names in the Python and C# generators.
  • Preserve original JSON field names through generated serialization metadata and dict keys.
  • Regenerate Python and .NET generated outputs and update serialization coverage.

Validation

  • npm --prefix .\nodejs test -- python-codegen.test.ts

Copilot AI review requested due to automatic review settings May 19, 2026 21:20
@stephentoub stephentoub requested a review from a team as a code owner May 19, 2026 21:20
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@stephentoub stephentoub force-pushed the stephentoub/duration-schema-handling branch from 9d6ea12 to 4d0ed58 Compare May 19, 2026 21:22
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR updates the Python and C# code generators to produce more idiomatic SDK member names for duration-valued schema properties by removing trailing Ms from the generated member name (while preserving the original JSON/wire property names via serialization metadata). This aligns the public SDK surface with the fact that these fields already map to timedelta/TimeSpan-like types, without changing the wire contract.

Changes:

  • Python codegen: detect duration-valued properties and strip a trailing Ms from the generated Python field name (snake_case), while keeping JSON keys unchanged.
  • C# codegen: apply the same Ms-stripping rule for duration-valued properties, including nullable wrappers, while keeping [JsonPropertyName("...Ms")] intact.
  • Regenerate Python/.NET generated outputs and extend Node-based codegen tests to validate name stripping + JSON-name preservation.
Show a summary per file
File Description
scripts/codegen/python.ts Adds duration-property detection and Ms suffix stripping when computing Python field names.
scripts/codegen/csharp.ts Adds duration-aware Ms suffix stripping for generated C# property names and improves nullable duration detection.
python/copilot/generated/session_events.py Regenerated output reflecting renamed duration members (e.g., ttft_msttft) while preserving JSON keys.
nodejs/test/python-codegen.test.ts Adds a test covering Ms stripping behavior and JSON-name preservation (Python + C#).
dotnet/test/Unit/SessionEventSerializationTests.cs Updates unit test to use the renamed .NET duration property.
dotnet/src/Generated/SessionEvents.cs Regenerated session event types with Ms-less duration property names and unchanged [JsonPropertyName].
dotnet/src/Generated/Rpc.cs Regenerated RPC types with Ms-less duration property names and unchanged [JsonPropertyName].

Copilot's findings

  • Files reviewed: 4/7 changed files
  • Comments generated: 1

Comment thread nodejs/test/session-event-codegen.test.ts
@github-actions

This comment has been minimized.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions
Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR makes a well-scoped, intentional distinction across the SDK implementations:

SDK Duration type Ms suffix treatment Rationale
Python timedelta Stripped (this PR) Native type makes suffix redundant
.NET TimeSpan Stripped (this PR) Native type makes suffix redundant
Go float64 / int64 Retained Raw numeric — suffix communicates the unit
Rust f64 / i64 Retained Raw numeric — suffix communicates the unit
Node.js number Retained Raw numeric — suffix communicates the unit

The approach is consistent: only SDKs that map duration fields to a dedicated native duration type get the suffix stripped, because in those cases the type already conveys "this is a duration" and the Ms suffix becomes noise. For languages using raw numbers, durationMs vs duration is the difference between self-documenting and ambiguous.

Additional consistency checks:

  • ✅ Wire format is unchanged in all SDKs — [JsonPropertyName("durationMs")] / obj.get("durationMs") preserve the original JSON names
  • ✅ Edge case handled correctly: URLMs is not stripped (the char before Ms is uppercase L), consistent in both Python and C# codegen
  • ✅ Test file renamed python-codegen.test.tssession-event-codegen.test.ts to accurately reflect its coverage of both Python and C# codegen

No cross-SDK consistency issues found.

Generated by SDK Consistency Review Agent for issue #1339 · ● 618.4K ·

@stephentoub stephentoub merged commit 584a790 into main May 19, 2026
35 checks passed
@stephentoub stephentoub deleted the stephentoub/duration-schema-handling branch May 19, 2026 22:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants