Closed adriangb closed 1 year ago
please review
Merging #46 (17a3987) into main (7058870) will increase coverage by
0.02%
. The diff coverage is100.00%
.
I'm a bit confused by why this is necessary and what it does.
I thought you could just do
TimeConfig { unix_timestamp_offset: Some(123), ..Default::default() };
And extra settings added to TimeConfig
wouldn't break your code?
Yes but then you have to force users to do that, and it’s not immediately obvious, eg in pydantic-core that’s not what we did. The builder pattern is meant precisely to address this situation.
No real performance impact:
name main ns/iter variable ns/iter diff ns/iter diff % speedup
compare_datetime_error_chrono 40 40 0 0.00% x 1.00
compare_datetime_error_iso8601 141 144 3 2.13% x 0.98
compare_datetime_error_speedate 10 10 0 0.00% x 1.00
compare_datetime_ok_chrono 166 166 0 0.00% x 1.00
compare_datetime_ok_iso8601 82 82 0 0.00% x 1.00
compare_datetime_ok_speedate 11 11 0 0.00% x 1.00
compare_duration_ok_iso8601 46 46 0 0.00% x 1.00
compare_duration_ok_speedate 31 32 1 3.23% x 0.97
compare_timestamp_ok_chrono 9 9 0 0.00% x 1.00
compare_timestamp_ok_speedate 8 7 -1 -12.50% x 1.14
date 3 3 0 0.00% x 1.00
dt_custom_tz 13 13 0 0.00% x 1.00
dt_naive 12 13 1 8.33% x 0.92
format_date 38 38 0 0.00% x 1.00
format_date_time 180 221 41 22.78% x 0.81
format_time 50 49 -1 -2.00% x 1.02
time 6 8 2 33.33% x 0.75
x_combined 92 96 4 4.35% x 0.96
I noticed while updating pydantic-core that despite efforts to avoid breaking changes adding a field to TimeConfig is indeed a breaking change. I'm adding this TimeConfigBuilder so that we can add more options down the road without a breaking change.
@samuelcolvin I realize we probably think we won't need to add any more options, if you're 100% sure of this please just close this, but I figured since we had 0 a week ago and now have 2...