Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98%. Comparing base (372daea) to head (a68c0ea).
⚠️ Report is 5 commits behind head on master.

@@          Coverage Diff           @@
##           master   #1724   +/-   ##
======================================
  Coverage      98%     98%           
======================================
  Files         515     515           
  Lines       24077   24090   +13     
======================================
+ Hits        23677   23695   +18     
+ Misses        400     395    -5     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

⚠️ Review: Non-blocking findings

✅ Breaking changes

• ✅ No public API changes -- TryParseFeetInches/ParseFeetInches signatures are unchanged; all new members (TryParseFeetInchesCombination, GetPossibleUnitSplitEndIndexes, TryParseSpecificUnit) are private.
• ✅ The precedence flip (combo-parse now attempted before the generic single-unit TryParse) is a behavior fix, not a break -- traced against several inputs (5 ft, 1,000′, 1e3') and confirmed the inchesPart.Length == 0 guard still lets feet-only/inches-only values fall through to the generic parser unchanged.

⚠️ Style and conventions

• ✅ Naming and structure (small private Try* helpers) match the surrounding custom-code conventions in Length.extra.cs.
• ⚠️ footRegex/inchRegex are now built with RegexOptions.IgnoreCase in addition to Singleline, whereas the previous implementation was case-sensitive. This is an unstated behavior change (e.g. an all-caps FT would now be accepted where it previously was not) -- worth confirming intent and noting it in the PR description, since it is not covered by a test either way.

⚠️ Code quality and best practices

• ✅ GetPossibleUnitSplitEndIndexes walking the string right-to-left and validating each candidate split via TryParseSpecificUnit is a clean, testable replacement for the old single monolithic anchored regex, and it correctly reuses the compiled footRegex/inchRegex across candidates instead of rebuilding per candidate.
• ⚠️ The leading minus sign is detected via a hardcoded ASCII hyphen check rather than NumberFormatInfo.NegativeSign for the given formatProvider. This matches the old regex's hardcoded behavior, so it is not a regression, but since this PR specifically hardens culture-aware parsing, it may be worth fixing in the same pass.
• ⚠️ Regex objects (footRegex, inchRegex) are still constructed with new Regex(...) on every call rather than cached/static -- pre-existing pattern, not introduced here, but flagging since this method now does more work per call (multiple candidate validations) than before.

⚠️ Test coverage

• ✅ Good targeted regression coverage for the reported bug: spaced/unspaced, multi-group (three feet groups), and negative variants using the apostrophe as both group separator and foot abbreviation.
• ⚠️ All new positive test cases happen to succeed on the first (rightmost) candidate produced by GetPossibleUnitSplitEndIndexes. No test forces the rightmost split to fail validation so the loop must fall back to an earlier split to succeed -- that fallback path is only exercised by already-invalid cases (e.g. two feet-apostrophes with nothing after, which fails on every candidate). Since exercising multiple split points is the core of this fix, a case requiring backtracking to a non-first candidate would strengthen confidence in the loop logic.

✅ Potential bugs / performance / security

• ✅ Traced the regex generated by QuantityParser.CreateRegexPatternForUnit (lazy value group, anchored at both ends) against the new grouping-vs-abbreviation test cases and confirmed the value/unit split resolves correctly given a group separator equal to the foot abbreviation.
• ✅ GetPossibleUnitSplitEndIndexes is O(n·k) (string length times abbreviation count) with no backtracking-prone regex construction, so no ReDoS or pathological performance concern for typical short user input.
• ✅ Malformed candidate substrings (e.g. an interior apostrophe swallowed into a candidate's numeric value) fail gracefully via QuantityValue.TryParse inside TryParseWithRegex rather than throwing, so a bad split point cannot crash the parse.

Not applicable to this PR: no new quantities/units added, and no changes to code generators or generated code.

✅ Review: No actionable findings

Focused, well-scoped fix for #817 (feet/inches parsing when ' is both the number group separator and the foot abbreviation). Only touches Length.extra.cs (custom code, no codegen/JSON changes) and adds tests.

✅ Breaking changes

• ✅ No public API signature changes (TryParseFeetInches/ParseFeetInches unchanged); new helper methods are private static.
• ⚠️ Behavior becomes strictly more permissive in two ways not covered by new tests: (1) footRegex/inchRegex now use RegexOptions.IgnoreCase (previously case-sensitive), so e.g. "1 FT 6 IN" will now parse where it previously failed — this actually makes it consistent with QuantityParser.CreateRegexForQuantity, which already uses IgnoreCase, so it's arguably fixing a pre-existing inconsistency rather than introducing a new one; (2) parsing order is now combination-first, single-value second (previously single-value was tried first). Both are correctness improvements, but since TryParseFeetInches/TryParse return bool rather than throw, it's very unlikely any caller depends on the old failure behavior.

✅ Style and conventions

• ✅ Follows existing file conventions (naming, private static helpers, file header retained).
• ✅ New using System.Collections.Generic; is required for IEnumerable<int>/IReadOnlyList<string> and correctly added.

New quantities or units

• N/A — no Common/UnitDefinitions/*.json changes.

Changes to generated code

• N/A — no generated code touched.

✅ Code quality & correctness

• ✅ The core algorithm — scanning candidate split points right-to-left by foot-abbreviation occurrence, then validating both sides against unit-specific anchored regexes — is a reasonable way to disambiguate "apostrophe as thousands separator" vs. "apostrophe as foot symbol" without a fragile single combined regex. I traced it by hand against all new and pre-existing test cases (including the InvalidData theory, e.g. "1' 1'", "1'1'", "1' 1") and it produces the expected results in each case.
• ✅ TryParseSpecificUnit's regex pre-check before calling the generic TryParse correctly prevents a candidate split from succeeding when the substring is actually some other unit (e.g. guards against "2 m" being accepted as the "inches" part).
• ⚠️ The "first candidate (rightmost) that validates wins" heuristic isn't proven exhaustive — it happens to work for every case in this PR's test suite, but it's worth a short comment in code (or in the PR description) noting the assumption it relies on (feet always precede inches, and the true unit boundary is the rightmost valid split), so future maintainers understand why this order was chosen rather than, say, leftmost-first.

✅ Performance

• ✅ No practical concern — feet/inches strings are short, so the extra per-candidate Regex.IsMatch + TryParse calls (O(number of apostrophe/foot-abbreviation occurrences)) are negligible compared to typical parsing costs.

✅ Test coverage

• ✅ Good regression coverage: spaced/unspaced, large numbers with multiple grouping separators, negative values, and a ToString()/ParseFeetInches() round-trip using a culture where ' is the group separator.
• ⚠️ Consider adding a case-insensitive unit test (e.g. "1 FT 6 IN") since that's now accepted as a side effect of adding RegexOptions.IgnoreCase, to make the behavior change explicit and intentional rather than incidental.

✅ Security concerns

• ✅ No concerns identified — regex patterns are built from a bounded, known abbreviation list, not user input, so no ReDoS risk beyond what already exists in QuantityParser.

⚠️ Review: Non-blocking findings

✅ Breaking changes

• ✅ No breaking changes. TryParseFeetInches/ParseFeetInches public signatures are unchanged; only the private combined-parsing logic in UnitsNet/CustomCode/Quantities/Length.extra.cs is reworked. As a side effect the fix also makes unit abbreviation matching case-insensitive (RegexOptions.IgnoreCase added), which is a strict widening of accepted input, not a break.

✅ New quantities or units

• ✅ N/A — no new quantities or units are introduced in this PR.

✅ Generated code

• ✅ N/A — no Common/UnitDefinitions/*.json or GeneratedCode/* changes; this is purely CustomCode.

✅ Code quality and correctness

• ✅ The right-to-left split search correctly resolves the ambiguity between the foot abbreviation (') and an apostrophe grouping separator, and I hand-traced it against the existing 'feet apostrophe twice' invalid cases (e.g. "1' 1'", "1'1'" in InvalidData) — the anchored per-part regex validation (TryParseSpecificUnit) correctly rejects the malformed inches segment ("1'" doesn't match the inch pattern), so those still fail to parse as before.
• ✅ Using matchEntireString: true (the new default) for footRegex/inchRegex is the right choice here since they're now used to validate whole candidate substrings via IsMatch, rather than being spliced into one bigger regex.
• ⚠️ GetPossibleUnitSplitEndIndexes combined with a TryParseSpecificUnit (regex + TryParse) call per candidate split is a step down in complexity from the original single anchored regex match — worst case is O(n) split candidates each doing a regex match/parse, which is fine for realistic input lengths but could add up for very long, adversarial strings (e.g. thousands of repeated '/ft substrings). Not a blocker given this is a formatting helper meant for short human-entered strings, but worth being aware of if this method is ever exposed to large untrusted input.

✅ Test coverage

• ✅ Good coverage of the actual bug: grouped feet (1'000'), grouped feet + inches with and without space, nested/double grouping (1'000'000' 2"), negative values, and a case where the inches part itself contains a group separator (1' 1'000").
• ✅ ParseFeetInches_WithConflictingGroupSeparator_RoundTripsFeetInchesToString is a nice addition — it verifies FeetInches.ToString() output can be round-tripped by the fixed parser under the same conflicting-culture conditions.
• ✅ The case-insensitivity fix ("1 FT 6 IN") is explicitly covered, tying the test back to the RegexOptions.IgnoreCase change.
• ⚠️ Consider also adding an invalid-input regression case under the apostrophe-grouping culture (e.g. a string with no unit at all, or one where none of the possible splits validate) to lock in the negative path for the new algorithm, mirroring the existing InvalidData theory used for en-US.

✅ Performance

• ⚠️ See the code quality note above — multiple Regex objects are now constructed per call (two, versus one previously) and multiple TryParse attempts may occur per input; likely negligible for typical short feet/inches strings but a measurable regression versus the old single-regex-match approach if this is ever called in a hot loop.

✅ Security

• ✅ No medium-or-higher severity concerns. The only theoretical issue is the algorithmic-complexity point above, which is low severity for this type of public parsing API and short expected input.

✅ Review: No actionable findings

Reviewed the fix for Length.TryParseFeetInches/ParseFeetInches handling apostrophe-as-group-separator cultures (fixes #817). Small, well-scoped, custom-code-only change — no generated code or JSON unit definitions touched.

✅ Breaking changes

• ✅ No breaking changes. Public signatures of TryParseFeetInches/ParseFeetInches are unchanged; only the private parsing internals were reworked.

✅ New quantities or units

• ✅ N/A — this PR doesn't add or modify any quantities/units.

✅ Changes to generated code

• ✅ N/A — no generator or GeneratedCode/ changes; this is entirely in UnitsNet/CustomCode/Quantities/Length.extra.cs.

✅ Code quality & correctness

• ✅ The new right-to-left, backtracking split search (GetPossibleUnitSplitEndIndexes + TryParseSpecificUnit) correctly resolves the feet-abbreviation/group-separator ambiguity. I hand-traced all the new TryParseFeetInches_WhenGroupSeparatorIsFootAbbreviation_* cases, including the trickiest one ("1' 1'000\"" → 84.33333333, which requires backtracking past a failed first candidate split), and the algorithm produces the expected results.
• ✅ Good decomposition into small private helpers (TryParseFeetInchesCombination, GetPossibleUnitSplitEndIndexes, TryParseSpecificUnit) each with a clear single responsibility, replacing a single hard-to-follow combined regex.
• ✅ Adding RegexOptions.IgnoreCase to both unit regexes is a deliberate, well-justified fix (covered by the new "1 FT 6 IN" case) that aligns feet/inches parsing with the case-insensitive unit parsing used elsewhere in the library.
• ⚠️ TryParseSpecificUnit runs unitRegex.IsMatch(str) and then calls TryParse(str, ...), which internally re-derives essentially the same value/unit split via its own regex — mild duplicated work per candidate split. Not a correctness issue, and input strings here are short, so this is a minor simplification opportunity rather than something blocking.
• ⚠️ The negative-sign handling (str.StartsWith("-", StringComparison.Ordinal)) only recognizes a literal ASCII hyphen, not culture-specific negative sign symbols. This mirrors the pre-existing behavior (previous regex also hardcoded -), so it's not a regression introduced by this PR, just worth noting as a pre-existing limitation.

✅ Performance

• ⚠️ GetPossibleUnitSplitEndIndexes is O(n·k) (n = string length, k = abbreviation count) and each yielded candidate triggers up to two Regex.Match calls — for pathological inputs (very long strings containing many repeated abbreviation characters, e.g. lots of ') this could add up. Given this API parses short, user-typed length strings (not bulk/untrusted payloads), this is a low-severity nit rather than a blocking concern.

✅ Test coverage

• ✅ Solid coverage: apostrophe-grouping combinations (spaced, unspaced, nested/multi-group, negative), an explicit invalid-string matrix, case-insensitivity, and a FeetInches.ToString()/ParseFeetInches() round-trip test under a conflicting-group-separator culture.
• ⚠️ Consider a symmetric case where the inch abbreviation (") collides with a culture's decimal/group separator, if that's a realistic scenario — not required for this fix since Culture-Dependent parsing of feet/inches #817 was specifically about the foot/group-separator collision, so not blocking.

✅ Security

• ✅ No medium+ severity concerns identified.

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.