Math ops for baml.time.* - #4366
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
📝 WalkthroughWalkthroughThis change adds arithmetic operators for ChangesTime arithmetic
Interface method code generation
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/baml_tests/baml_src/ns_time/time.baml (1)
323-329: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a timezone-offset transition case.
The case at Line 325 starts after the local offset transition.
later.timezone()and(later - base)can pass if arithmetic retains the timezone identifier but does not resolve the new local offset.Add a case that adds one hour to
2024-03-10T01:00inAmerica/New_York. Assert thatlater.hour()is3.Proposed test
+test "zoned_arithmetic_applies_timezone_offset_transition" { + let base = baml.time.ZonedDateTime.from_components( + "America/New_York", 2024, 3, 10, hour = 1 + ); + let later = base + baml.time.Duration.from_hours(1n); + assert.equal(later.timezone(), "America/New_York"); + assert.equal(later.hour(), 3); + assert.equal((later - base).to_hours(), 1n) +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_tests/baml_src/ns_time/time.baml` around lines 323 - 329, Extend the zoned arithmetic coverage in test "zoned_arithmetic_preserves_the_timezone" with a base at 2024-03-10 01:00 in America/New_York, add one hour using baml.time.Duration.from_hours, and assert later.hour() equals 3 to verify the DST offset transition is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@baml_language/crates/baml_tests/baml_src/ns_time/time.baml`:
- Around line 323-329: Extend the zoned arithmetic coverage in test
"zoned_arithmetic_preserves_the_timezone" with a base at 2024-03-10 01:00 in
America/New_York, add one hour using baml.time.Duration.from_hours, and assert
later.hour() equals 3 to verify the DST offset transition is resolved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4eb7252b-bbea-40fb-815d-c43bad45363d
⛔ Files ignored due to path filters (10)
baml_language/crates/baml_cli/src/snapshots/baml_cli__describe_command_tests__render_builtin_package_listing.snapis excluded by!**/*.snapbaml_language/crates/baml_cli/src/snapshots/baml_cli__describe_render__tests__renders_builtin_class_with_impls.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/baml_src/time.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____03_ppir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_5_mir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____04_tir.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/snapshots/compiles/__baml_std__/baml_tests__compiles____baml_std____06_codegen.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/src/compiler2_tir/snapshots/baml_tests__compiler2_tir__phase5__snapshot_baml_package_items.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
baml_language/crates/baml_builtins2/baml_std/baml/ns_time/duration.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_time/instant.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_time/plaindatetime.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_time/plaintime.bamlbaml_language/crates/baml_builtins2/baml_std/baml/ns_time/zoneddatetime.bamlbaml_language/crates/baml_tests/baml_src/ns_time/time.baml
Binary size checks passed✅ 7 passed
Generated by |
We don't generate interfaces and they can result in name conflicts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
baml_language/crates/baml_project/src/client_codegen.rs (1)
206-228: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winCover the owning-class filtering path with a Rust unit test.
The new branch removes methods before they enter
pending_methods.test_interface_and_implements_methods_do_not_reach_free_function_poolchecks only the free-function filter, so it would still pass if this branch were removed. Add a test in the existingmod teststhat asserts interface implementation methods are absent from the owning class’sstatic_methodsandinstance_methods.As per coding guidelines: “
**/*.rs: Prefer writing Rust unit tests over integration tests where possible” and “Always runcargo test --libif you changed any Rust code.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baml_language/crates/baml_project/src/client_codegen.rs` around lines 206 - 228, Add a Rust unit test in the existing tests module covering the owning-class filtering branch before methods enter pending_methods. Assert that interface implementation methods are absent from the owning class’s static_methods and instance_methods, while preserving the existing free-function test and behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baml_language/sdk_tests/harness_setup/src/csharp.rs`:
- Around line 466-470: Update the C# generation expectation for Csv/CsvRows.g.cs
in the surrounding test setup to assert that the removed Iter, IterAsync, Next,
and NextAsync members are absent, while retaining the existing Reader presence
assertion. Apply the same negative checks to each affected generated C# surface
represented by this test.
---
Nitpick comments:
In `@baml_language/crates/baml_project/src/client_codegen.rs`:
- Around line 206-228: Add a Rust unit test in the existing tests module
covering the owning-class filtering branch before methods enter pending_methods.
Assert that interface implementation methods are absent from the owning class’s
static_methods and instance_methods, while preserving the existing free-function
test and behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4de7e3ca-e37f-43a4-bd6c-0b22625154c5
⛔ Files ignored due to path filters (2)
baml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded.snapis excluded by!**/*.snapbaml_language/crates/baml_tests/tests/bytecode_format/snapshots/bytecode_format__bytecode_display_expanded_unoptimized.snapis excluded by!**/*.snap
📒 Files selected for processing (2)
baml_language/crates/baml_project/src/client_codegen.rsbaml_language/sdk_tests/harness_setup/src/csharp.rs
meefs
pushed a commit
to meefs/baml
that referenced
this pull request
Aug 15, 2026
…L#4446) ## Summary - Fixes the deterministic C# verification failure in the baml_language 0.17.0 production release run: https://github.com/BoundaryML/baml/actions/runs/31857280879 - Aligns the phase-12 package consumer with the generated C# surface established by BoundaryML#4366, which intentionally omits methods inherited through BAML interface implementations. - Keeps runtime coverage for the C# CSV methods and resource fields that are generated. ## Root cause The phase-12 fixture is normally ignored because of B-1059, but the production full-surface package verifier compiles and runs it. Its stale calls to CsvReader.Iter/Next and CsvRows.Iter/Next no longer compile after BoundaryML#4366 removed interface-implementation methods from generated class method collections. ## Validation - cargo nextest run -p sdk_test_csharp --all-features --run-ignored only -E test(test_phase12_executes_native_typed_resource_apis_lifetimes_and_state) - dotnet build sdk_tests/crates/csharp/phase12_resources/Phase12Resources.csproj --configuration Release --no-restore - git diff --check ## Release follow-up After this merges and post-merge canary CI succeeds, 0.17.0 must be dispatched from the new immutable baml-language source tag. This PR does not publish or dispatch the release. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Updated C# CSV resource validation to verify headers, reader availability, and the initial reading position. * Simplified resource cleanup validation to ensure the active CSV reader is properly closed. * Removed checks for iterator identity, typed row decoding, and raw record access. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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
1 participant
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.
Durationfrom time quantitiesDurationbyintorbigintDurationbyDurationDurationDurationSummary by CodeRabbit
New Features
Bug Fixes
Tests