fix handling of rounding to 12 inches in FeetInches

[Applesauce314]

updates the string outputs of FeetInches to propagate the rounding of inches up to feet if the inches are rounded to 12.
i.e 3.9999 ft in feet inches is 3 feet 11.9988 inches.
ToString will round the 11.9988 to 12 and output 3 ft 12 in, which is incorrect.
ToArchitecturalString will do the same if the denominator is is not small enough to result is a fractional part that is not rounded to 1.

this PR updates these methods to propagate the rounding of these functions up to the feet part while resetting the inches to 0.

i.e. 3 ft 11.9988 in becomes 4 ft 0 in instead of 3 ft 12 in.

the current behavior is unexpected for a mixed radix system, and this PR implements the expected default behavior.
(for an example of a similar situation, if rounding 09:59:50 to the nearest minute, you would get 10:00, not 09:60)

this issue had been previously discussed in #158, but does not provide a way to configure the behavior as mentioned there.

[Applesauce314]

it looks like this pipeline failure is unrelated to my change, anything i need to do?

[claude]

Code Review

Summary

Fixes an edge case where rounding inches to 12 would display as e.g. "3 ft 12 in" instead of "4 ft 0 in". The core logic is correct — overflow from rounding is caught and carried into the next foot.

Bug: ToArchitecturalString still uses Feet in the zero-guard

The ToString() fix correctly uses the local feet variable throughout. However, ToArchitecturalString still has the original if (Feet == 0) check at line 236 (unchanged by this PR):

if (Feet == 0)          // ← still reads the original property
{
    return inchPart.ToString();
}

return $"{feet}' - {inchPart}";   // ← correctly uses local variable

If someone creates a FeetInches(0, 11.x) where rounding pushes inchTrunc to 12, the code will:

1. Correctly set feet = 1, inchTrunc = 0
2. Build inchPart as "0\""
3. But then if (Feet == 0) is still true, so it returns just "0\"" instead of "1' - 0\""

if (Feet == 0) should be changed to if (feet == 0).

Style

if(inches == InchesInOneFoot) is missing a space after if. The rest of the codebase uses if (condition).

Tests

No tests are added for the boundary case (e.g., a length that rounds to exactly 12 inches). It would be good to add a test like:

// Very close to 4 ft 0 in — rounds up from 3 ft 12 in
Assert.Equal("4 ft 0 in", Length.FromFeetInches(3, 11.9999).FeetInches.ToString(CultureInfo.InvariantCulture));

// ToArchitecturalString equivalent
Assert.Equal("4' - 0\"", Length.FromFeetInches(0, 11.9999).FeetInches.ToArchitecturalString(1));

Minor

The new comment in ToString() is a bit run-on. The existing comment above already explains why inches are rounded; the new comment just needs to note the carry logic. Something like:

// Carry rounded inches that reach 12 into the next foot.

[claude]

Code Review

Overview: Fixes a bug where rounding inches to the nearest integer could produce X ft 12 in instead of (X+1) ft 0 in (e.g. 3.9999 ft → 3 ft 12 in instead of 4 ft 0 in). The fix is logically correct.

✅ What's good

• The fix correctly applies carry-over logic in both ToString and ToArchitecturalString
• The analogy to clock rounding in the PR description is apt and well-explained
• Uses the existing InchesInOneFoot constant rather than a magic number

⚠️ Missing tests

The PR modifies behavioral logic but adds no tests to cover the fixed case. Please add regression tests to UnitsNet.Tests/CustomCode/LengthTests.cs:

// ToString rounding carry-over
[Theory]
[InlineData(3.9999, "4 ft 0 in")]   // rounds up
[InlineData(3.999, "3 ft 12 in")]   // depending on exact threshold
public void FeetInches_ToString_RoundsInchesTo12_CarriesOverToFeet(...)

// ToArchitecturalString carry-over
[InlineData(3, 11.9999, 1, "4' - 0\"")]
public static void ToArchitecturalString_RoundsInchesTo12_CarriesOverToFeet(...)

🐛 Potential issue in ToArchitecturalString

The overflow check for inchTrunc == InchesInOneFoot is placed correctly (after the numerator overflow block), but there's a subtle edge case: if Inches itself is already >= 12.0 (which should not occur due to how Inches is computed, but defensive code may help). A comment explaining why inchTrunc can only reach 12 via the numerator overflow path would aid future readers.

Minor style

• Trailing whitespace on blank line after the new if block in ToString (line with just spaces)
• Missing space before ( in if(inches == InchesInOneFoot) — should be if (inches == InchesInOneFoot) per C# conventions

The fix itself is correct — just needs tests before merging.

[claude]

Code Review

Overview

Fixes ToString() and ToArchitecturalString() to carry a rounded/truncated 12-inch value up to the feet column, matching standard mixed-radix display conventions.

Correctness

The fix is correct for both paths:

ToString():

var feet = Feet;
var inches = Math.Round(Inches);
if (inches == InchesInOneFoot)
{
    feet++;
    inches = 0;
}

Handles the case where Inches (e.g. 11.9988) rounds to 12.

ToArchitecturalString():

if (numerator == fractionDenominator) { inchTrunc++; numerator = 0; }
if (inchTrunc == InchesInOneFoot) { feet++; inchTrunc = 0; }

The chain is correct: fraction carry -> inch carry -> feet carry.

Missing Tests

No tests are added for the bug fix. Please add test cases for the boundary values, e.g.:

• 3.9999 ft -> "4 ft 0 in" (not "3 ft 12 in")
• ToArchitecturalString(16) for the same value

Without tests this edge case may regress.

Style

Minor: missing space in if(inches == InchesInOneFoot) - should be if (inches == InchesInOneFoot) per project convention.

Summary

The logic is correct, but the PR needs test coverage for the fixed edge case.

[claude]

Code Review

Bug fix: propagate inch rounding to feet in FeetInches — this is a valid and well-understood issue (analogous to 09:60 → 10:00 in time formatting).

What the fix does

• ToString: rounds inches, then if they reach 12 bumps feet and resets inches to 0.
• ToArchitecturalString: applies the same carry-over after the existing numerator-rollover logic, so the ordering is correct (fraction rolls into whole inches, then whole inches roll into feet).

Concerns

1. No regression test added. The repro case from the PR description (3.9999 ft → "4 ft 0 in" instead of "3 ft 12 in") should have a test. A test method in the custom test file would be easy and would prevent future regressions.

2. Negative lengths are not handled. If Inches is -11.9988 and Math.Round(-11.9988) = -12, the check inches == InchesInOneFoot won't fire (−12 ≠ 12) and the output will still show "−3 ft −12 in". This may be pre-existing behaviour, but worth a note or a follow-up issue.

3. Minor: the new comment on ToString is slightly long and describes what the code does rather than why. The why is already in the PR description; an inline comment doesn't need to repeat it.

Overall the logic is correct for positive values. Please add at least one test before merging.

[claude]

PR Review: fix handling of rounding to 12 inches in FeetInches

Overview: Fixes both ToString() and ToArchitecturalString() to carry a rounding overflow of 12 inches up to the feet part rather than emitting "3 ft 12 in". The analogy to 09:60 vs 10:00 in the PR description is a good way to frame this.

---

Positive

• The fix is correct for positive lengths in both display paths
• ToArchitecturalString handles the two-step carry correctly: numerator == fractionDenominator → inchTrunc++ first, then inchTrunc == 12 → feet++
• Using the existing InchesInOneFoot constant rather than a magic 12 is clean

---

Concerns

Negative lengths not handled:
For a negative length like -3.9999 ft:

• Feet = -3 (truncates toward zero)
• Inches = (-3.9999 + 3) * 12 = -11.9988
• Math.Round(-11.9988) = -12
• The check if (inches == InchesInOneFoot) tests if (-12 == 12) → false, so the carry is skipped and "-3 ft -12 in" is still emitted.

This was pre-existing behavior, so it's out of scope for this PR, but worth adding a note or a follow-up issue.

Missing regression tests:
The PR has no new unit tests. Given that this is a display edge-case fix, it's easy to regress silently. A test covering at least:

• Length.FromFeetInches(3, 11).FeetInches.ToString() → "3 ft 11 in" (no carry)
• A length just under 4 ft that rounds to "4 ft 0 in"
• ToArchitecturalString equivalent

would make this much safer.

---

Minor

• The comment added inside ToString() is a bit long; the existing comment above already explains the rounding rationale. Could be trimmed.

---

Overall: The fix is correct for the common (positive) case. Please add test coverage before merging to prevent regression.