Round a fractional offset given as a string

[youdie006]

The inconsistency

date_zone_to_diff returns a Rational for a fractional-hour zone with more
than two digits (ext/date/date_parse.c:529-535), and the comment just above it
says that is intended — "no over precision for offset; 10**-7 hour = 0.36
milliseconds should be enough" (:506-508).

offset_to_sec's string branch threw that away: if (!FIXNUM_P(vs)) return 0;
at ext/date/date_core.c:2639.

Its two sibling branches in the same function do the opposite — they round and
warn. T_FLOAT at :2590-2592 and T_RATIONAL at :2621-2623 both emit
rb_warning("fraction of offset is ignored").

So one quantity, three ways, two answers:

DateTime.new(2001,2,3, 0,0,0, '+00.123')                #=> offset 0     "invalid offset is ignored"
DateTime.new(2001,2,3, 0,0,0, 442.8/86400)              #=> offset 443   "fraction of offset is ignored"
DateTime.new(2001,2,3, 0,0,0, Rational(2214, 5*86400))  #=> offset 443   "fraction of offset is ignored"

442.8 seconds either way. The string form — the only one a user actually types —
is the one that silently becomes +00:00.

Not currently tested

git ls-files -z test | xargs -0 grep -nE "['\"][-+][0-9]{2}\.[0-9]+['\"]" returns
nothing: no test passes a dotted zone as an offset argument. The .123 hits in
the suite are sub-second fractions of the time ('19990523235521.123456+0900'),
not zones, and grep -n 'fraction of offset' test/ finds nothing.

The one fractional zone that is tested, '[-9.50]' at test_date_parse.rb:137-138,
has two digits, so it takes date_zone_to_diff's n <= 2 Integer path
(date_parse.c:524-528) — already consistent, and untouched by this. The Rational
path is what was untested.

The change

Round it, exactly as the sibling branches do. k_rational_p, f_round,
f_eqeq_p and rb_warning are all already used in this file.

The k_rational_p guard matters: date_zone_to_diff returns Qnil for a zone it
rejects, and without the guard that nil reaches f_round.

Not a widening

'+24:00' and '+99:00' still give 0 — date_zone_to_diff rejects them before
this code runs. '+00:00' → 0, '+01.5' → 5400, '+9.50' → 34200, '+00.1' →
360, '+00.12' → 432, '+23:59:59' → 86399 are all unchanged. The
n < -DAY_IN_SECONDS || n > DAY_IN_SECONDS check at :2642 still runs on the
rounded value, and '+23.9999999' rounds to exactly 86400, which the inclusive
guard accepts — consistent with Rational(1,1) being legal per the existing
test_civil__offset at :196-197.

Direction

I want to be straight about the tension here. Read shallowly, the recent commits
on these files are a tightening trend (#183's range check, #188's commercial-week
validation, c98d85d verifying argument classes), which would argue for making the
string branch keep rejecting.

I do not think that is the right read. #183's own commit message frames its defect
as "the Integer 2 is rejected, but Rational(2,1) is the same quantity" — the
principle is consistency between representations of one quantity, which is exactly
this. And rounding is the long-standing behaviour here: rb_warning("fraction of offset is ignored") already exists at :2592, :2623 and :3349. The string
branch is the deviation, not the rule.

If you would rather go the other way and make Float and Rational reject too, that
is a coherent position and I am happy to write that instead.

Verification

• Full suite on CRuby 3.2.11: 148 tests, 162616 assertions, 0 failures, 0
  errors. Pristine control run: same 148 tests with exactly one failure, mine —
  so nothing else changes behaviour.
• TruffleRuby row verified, not inferred. That row was my one open question,
  since TruffleRuby ships its own date. It does compile this C extension via
  Sulong, and I ran it: with master's code '+00.123' gives 0 and warns "invalid
  offset is ignored"; with this patch it gives 443 and warns "fraction of offset is
  ignored", while '+24:00'/'+99:00' stay 0 and '+01.5'/'+23:59:59' are
  unchanged. So the truffleruby job is fixed by this too rather than broken by it.
• Mutation checks: truncating instead of rounding gives 442 and fails; dropping the
  warning fails; dropping the k_rational_p guard raises
  NoMethodError: undefined method 'round' for nil on the '+24:00' row.

One thing I deliberately left out

DateTime.parse('2001-02-03T00:00:00+00.123') goes through dt_new_by_frags,
which does of = NUM2INT(t) at date_core.c:8519 and truncates to 442. So
after this change DateTime.parse and DateTime.new differ by one second on the
same zone string.

That is a real but separate inconsistency in a different function, and bundling it
would make this patch harder to judge. Happy to follow up. (Same spot also narrows
before its range check at :8520, the inverse of #183's principle — currently
unreachable, since date_zone_to_diff caps output near 3.6e8.)

Disclosure

I used an AI assistant to help find and prepare this change. I reviewed and tested
it myself, and the outputs above are from runs I performed.

[jeremyevans]

I think it would be better to deprecate fractional hour support in offsets, and only handle full hour or hour:minute. From some brief searching, I couldn't find any popular programming language support for fractional hour offsets, and Ruby's Time does not support it either.

[youdie006]

That makes sense to me, and your premise checks out — Time does reject fractional-hour offsets. I measured every offset spelling on ruby 3.2.11 / date 3.3.3, offsets shown in seconds:

zone	Time.new	DateTime.civil
+05	18000	18000
+0530	19800	19800
+05:30	19800	19800
+05:30:30	19830	19830
+053030	19830	19830
+05.5	ArgumentError	19800
+05:30.5	ArgumentError	19800
+00.123	ArgumentError	0

One adjustment to the scope, and it is the reason I am posting the table rather than just agreeing: "only full hour or hour:minute" would also drop +HH:MM:SS and +HHMMSS, and those are not part of the divergence — Time accepts both and the two libraries already return identical values for them. What Time rejects is specifically the decimal-fraction form, +HH.f and +HH:MM.f. So the deprecation target would be the decimal point, not the seconds field.

The other thing worth noting is the last row. +00.123 does not currently give a fractional offset — it gives 0, silently. Date._parse produces (2214/5) (442.8s) and offset_to_sec then drops it on the floor because it is not a Fixnum. So the status quo is wrong under either plan; this PR treats it as a rounding bug, and your plan treats it as an input that should never have been accepted. I think yours is the better framing, and it also removes the odd case where +05.5 is silently a synonym for +05:30.

Happy to redo this PR as the deprecation. Two questions on shape:

1. Deprecate for one release and then raise Date::Error, matching what we settled on for the RFC 3339 separator in Only allow "T" or a space as the RFC 3339 separator time#85, or raise straight away since the current answer for +00.123 is already wrong?
2. Should the warning fire for every decimal-fraction offset, including +05.5 where the value we return happens to be correct, or only where the fraction is actually lost?

One implementation note carried over from #191: rb_warning_category_enabled_p is not usable from an extension — it is in no public header and nm -D finds it 0 times in libruby on 3.4, and building with it fails on both 2.7 and 3.4. So the warning would go through rb_category_warn, which ext/date/extconf.rb:10 already probes and date_core.c:4690 already shims.

[jeremyevans]

Deprecation similar to #191 is best.

In terms of the warning, we should warn any case where a parsing the fractional offset hour affects the resulting return value. If the fractional offset part is already ignored, there is no need to warn, because the behavior will change to ignoring the value.

[youdie006]

Understood on the shape — deprecation like #191. Before I write it, one thing your rule decides differently depending on which API is asked, so I would rather show you than guess.

Measured on 3.2.11, decimal-fraction offsets only (the +HH:MM forms are untouched):

zone	Date._parse[:offset]	DateTime.civil(...,s).offset	with the fraction removed	changes the value?
+05.0	18000	18000	18000	no
+05.000	18000	18000	18000	no
+05.5	19800	19800	18000	yes
+00.5	1800	1800	0	yes
-05.5	-19800	-19800	-18000	yes
+05.123	(92214/5)	0	18000	yes
+00.123	(2214/5)	0	0	depends

The last two rows are the awkward ones. Three or more fractional digits make date_zone_to_diff return a Rational (date_parse.c:524-535), and offset_to_sec drops any non-Fixnum on the floor (date_core.c:2639), so DateTime.civil already answers 0 — while Date._parse hands the Rational straight back.

So for +00.123: DateTime.civil gives 0 today and would give 0 after the change, which is your "already ignored, no need to warn" case. But Date._parse gives (2214/5) today and would give 0, which is a changed return value and by the same rule should warn.

Two ways to resolve it:

1. Warn in date_zone_to_diff whenever the fractional digits are non-zero. Simple, one site, catches _parse too. Cost: +00.123 warns even though DateTime.civil users see no change.
2. Warn in offset_to_sec only when the value actually differs there. Matches your sentence exactly for DateTime.civil, but Date._parse/Date._rfc3339 and friends never reach offset_to_sec, so the syntax would go un-deprecated on the parse APIs and then change under them silently.

I lean towards (1) — the thing being deprecated is the +HH.f spelling, and _parse is where people meet it — but (2) is the literal reading of what you wrote. Which do you want? I will send the patch either way, plus the Warning[:deprecated] mechanism from #191.

[jeremyevans]

It's really bad to emit a deprecation warning when the behavior will not change. You are not limited to adding a deprecation warning in a single place. You can add a warning in offset_to_sec and add a separate warning for Date._parse and related methods. The principle remains the same, warn if behavior will change, do not warn if behavior will not change.

[youdie006]

Reworked to deprecation only; nothing returns a different value than before.

Two warnings, gated separately as you described. date_zone_to_diff keeps its signature and warns whenever the fractional hour is non-zero, which covers Date._parse and the other parse entry points, since they store the fraction and so will change when it is ignored. offset_to_sec now calls a new date_zone_to_diff_frac, which reports the fraction instead of warning, and warns only where the offset it returns today differs from the whole hour alone.

That second gate is what splits the two cases you asked about. Measured on master, then on this branch:

zone	Date._parse[:offset]	warns	DateTime.civil offset (s)	warns
+00.5	1800	yes	1800	yes
+00.123	(2214/5)	yes	0	no
-00.123	(-2214/5)	yes	0	no
+01.123	(20214/5)	yes	0	yes
+01.0	3600	no	3600	no
+00:30	1800	no	1800	no

+00.123 is quiet on the civil side because offset_to_sec already drops the whole zone there, so ignoring the fraction lands on the same 0. +01.123 is not quiet, because dropping the fraction gives 3600 rather than the 0 it returns now. The colon forms have no fractional hour and never warn.

Writing the test for the quiet rows caught something worth mentioning: DateTime.civil(..., "+00.123") already emits invalid offset is ignored, so the assertion is that the new deprecation does not appear, not that nothing is warned.

rake test equivalent: 147 tests 0 failures on master, 149 tests 0 failures here.

---

Disclosure: written with the help of Claude (an AI assistant). The tables are from runs on my machine.

[jeremyevans]

Suggested change

	" and will be ignored");
	" and will be ignored in a future version");

Similar change for the other warning.

[jeremyevans]

This should warn, because while the :offset entry returned may not change, the :zone entry should change to remove the .0

[jeremyevans]

Is there a reason for this approach instead of assert_warning?

[jeremyevans]

Add the following block up to near the top of the file (copied from date_core.c):

#ifndef HAVE_RB_CATEGORY_WARN
#define rb_category_warn(category, fmt) rb_warn(fmt)
#endif

then switch to:

Suggested change

	rb_warning("fraction of hour in a zone offset is deprecated"
	rb_category_warn(RB_WARN_CATEGORY_DEPRECATED, "fraction of hour in a zone offset is deprecated"

Similar change for the other warning, except there you would just move the block up in the same file.

[jeremyevans]

Crazy, this supports , in addition to .

We should do a manual check that removing this else if branch only breaks the deprecated behavior tests and no other tests.

[jeremyevans]

There should be tests added for Date._strptime, which should have appropriate warnings