Programatically generate partial final fuzzy entries / inner segement.#137
Conversation
This would introduce quite some new entries: e.g. we -> wei, wen, weng The rule to partial final entries: it's not valid, if final of partial final is longer than 2, its (length-1) prefix also need to be invalid pinyin: e.g. sua -> su is valid, so not added to partial final. e.g. ton -> to is invalid, so ton added to partial final. With that we may add more inner fuzzy, so simply generate inner fuzzy programatically. Fix #136
📝 WalkthroughWalkthroughChangesPartial-final matching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PinyinMapV2
participant InnerSegmentMapV2
participant PinyinEncoder
participant CorrectionTests
PinyinMapV2->>PinyinMapV2: Add PartialFinal entries
PinyinMapV2->>InnerSegmentMapV2: Generate validated inner segments
InnerSegmentMapV2->>PinyinEncoder: Expand user pinyin
PinyinEncoder->>PinyinEncoder: Gate single trailing "n"
CorrectionTests->>PinyinEncoder: Assert "to" and "ton" parses
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/libime/pinyin/pinyinencoder.cpp (1)
325-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented heuristic; test coverage for this branch is unclear.
This new gating condition (
innerSeg.second == "n") suppresses trivial "X+n" inner-segment splits now produced by the algorithmically-derivedgetInnerSegmentV2(e.g. "pin"→"pi"+"n", "lin"→"li"+"n") unless more input follows or it's the sole candidate. The rest of this function is heavily commented to explain similarly subtle heuristics (e.g. "check fuzzy seg", "zhuni -> zhu ni"), but this block has none.Separately, tracing the newly-added "to"/"ton" tests suggests neither actually reaches this branch (no inner-segment entry exists for "ton" since "to" isn't a valid complete pinyin), and I don't see a test exercising the
nNextSize == 2path together with an"n"-ending inner segment (e.g. a case like "lin" or "wan" underInnerShortwhere two candidate sizes are produced). Worth double-checking test coverage actually reaches this specific condition before relying on it.🤖 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 `@src/libime/pinyin/pinyinencoder.cpp` around lines 325 - 332, Document the heuristic in the inner-segment handling around innerSeg.second == "n", explaining why trivial X+n splits are rejected unless additional input follows or this is the sole candidate. Verify and add focused coverage that reaches this condition, including an n-ending case with nNextSize == 2 such as “lin” or “wan” under InnerShort; do not rely on the existing “to”/“ton” tests unless they actually exercise the branch.
🤖 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 `@src/libime/pinyin/pinyinencoder.cpp`:
- Around line 325-332: Document the heuristic in the inner-segment handling
around innerSeg.second == "n", explaining why trivial X+n splits are rejected
unless additional input follows or this is the sole candidate. Verify and add
focused coverage that reaches this condition, including an n-ending case with
nNextSize == 2 such as “lin” or “wan” under InnerShort; do not rely on the
existing “to”/“ton” tests unless they actually exercise the branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8077d64e-b1fd-4ac5-9fc5-f461ce371c86
📒 Files selected for processing (3)
src/libime/pinyin/pinyindata.cppsrc/libime/pinyin/pinyinencoder.cpptest/testpinyinencoder.cpp

This would introduce quite some new entries:
e.g. we -> wei, wen, weng
The rule to partial final entries:
it's not valid, if final of partial final is longer than 2, its (length-1)
prefix also need to be invalid pinyin:
e.g. sua -> su is valid, so not added to partial final.
e.g. ton -> to is invalid, so ton added to partial final.
With that we may add more inner fuzzy, so simply generate inner fuzzy
programatically.
Fix #136
Summary by CodeRabbit
New Features
Bug Fixes