Skip to content

feat(pinyin): convert first of two third tones to second tone - #356

Open
luojiyin1987 wants to merge 6 commits into
zh-lx:mainfrom
luojiyin1987:feat/third-tone-sandhi
Open

feat(pinyin): convert first of two third tones to second tone#356
luojiyin1987 wants to merge 6 commits into
zh-lx:mainfrom
luojiyin1987:feat/third-tone-sandhi

Conversation

@luojiyin1987

@luojiyin1987 luojiyin1987 commented Aug 22, 2026

Copy link
Copy Markdown
Contributor

Refs #286

Summary

  • Convert the first syllable of exactly two consecutive third tones to the second tone.
  • Keep runs longer than two third tones unchanged for now.
  • Treat punctuation and non-Chinese characters as boundaries.
  • Sync originPinyin so toneType: num reflects the change.
  • Apply the rule after custom pinyin resolution.
  • Preserve base tones when toneSandhi: false.

Examples

Input Output
你好 ní hǎo
你好,很好 ní hǎo , hén hǎo
我很好 wǒ hěn hǎo (unchanged)
你好 with toneType: num ni2 hao3

Out of scope

Triple third-tone runs need word boundary information. A follow-up design will cover 我很好,纸老虎,展览馆.

Verification

  • pnpm test
  • pnpm --filter pinyin-pro lint
  • pnpm build && pnpm size

Performance

The first CI benchmark run (commit f0cfde6) reported a large regression signal. That run predates the fix below and is outdated.

  • bdf8bfb replaced the getNumOfTone() classifier in the scan with a single lightweight regex test. The classifier did an array split plus up to six regex checks per character.
  • After the fix, three local rounds of pnpm compare measured 3.84 / 3.99 / 3.76 ms vs master 3.34 / 3.39 / 3.16 ms. The residual ~0.5 ms per full-text conversion (~15-19%) is the inherent cost of one extra pass over the result list while toneSandhi defaults to true.
  • The latest CI comparison comment shows the two versions within 5% (5.72 ms vs 5.80 ms), which is within noise.

Bundle size: +279 B vs master (+0.09%).

@github-actions

github-actions Bot commented Aug 22, 2026

Copy link
Copy Markdown

📦 CDN vs Local 完整对比


> pinyin-pro-repository@1.0.0 compare /home/runner/work/pinyin-pro/pinyin-pro
> pnpm --filter pinyin-pro compare


> pinyin-pro@3.29.3 compare /home/runner/work/pinyin-pro/pinyin-pro/packages/pinyin-pro
> ts-node --project scripts/tsconfig.json scripts/benchmark/compare.ts


========================================
   线上 vs 本次 完整对比
========================================

📦 文件体积对比
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
正在读取当前分支文件...
当前分支文件大小: 317.22 KB
正在获取 master 文件大小...
master 文件大小:  316.94 KB

⬆️ 当前分支文件比 master 文件大 285 B (+0.09%)

✈️ 速度对比
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
正在加载当前分支版本...
✓ 当前分支版本加载成功
正在下载 master 版本...
✓ master 版本下载并加载成功

100次运行平均值
当前分支版本: 4.65ms /次
master 版本:  4.52ms /次

✅ 速度基本相同 (差异 < 5%)

========================================


Updated at Sat, 22 Aug 2026 03:15:48 GMT


💡 提示: 此评论会在每次推送新提交时自动更新

@codecov

codecov Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f69ec01) to head (30b438f).

Additional details and impacted files
@@            Coverage Diff            @@
##              main      #356   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           29        29           
  Lines        26315     26364   +49     
  Branches       552       565   +13     
=========================================
+ Hits         26315     26364   +49     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@luojiyin1987
luojiyin1987 marked this pull request as ready for review August 22, 2026 03:14
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

feat(pinyin): apply third-tone sandhi for exactly two consecutive 3rd tones

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

Grey Divider

AI Description

• Apply Mandarin third-tone sandhi for exactly two consecutive third-tone syllables when enabled.
• Treat punctuation/non-Chinese characters as boundaries; keep 3+ third-tone runs unchanged.
• Update docs and tests to cover behavior, formats, and custom pinyin interactions.
Diagram

graph TD
  A["Input text"] --> B["Custom pinyin resolve"] --> C["Tone sandhi middleware"] --> D["Output formatting"]
  C --> E{“toneSandhi” on?}
  E -->|No| D
  E -->|Yes| F["Yi/Bu rules"] --> G["3rd-tone pair scan"] --> H["Convert 1st to 2nd"] --> D

  subgraph Legend
    direction LR
    _step["Processing step"] ~~~ _dec{"Decision"}
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Apply sandhi on a normalized tone-number representation
  • ➕ Avoids diacritic-specific regex and mapping tables
  • ➕ More uniform across toneType outputs (symbol/num/none)
  • ➕ Potentially clearer correctness for edge cases (e.g., ü, ê, m/n finals)
  • ➖ Requires refactoring internal representation and conversion points
  • ➖ Higher risk change that could affect existing formatting and custom dict behavior
2. Generalize third-tone sandhi to handle 3+ runs using word boundaries
  • ➕ Matches real Mandarin sandhi behavior for phrases like 我很好/纸老虎/展览馆
  • ➕ Could provide more linguistically correct output
  • ➖ Needs word segmentation / boundary inference, which is non-trivial
  • ➖ Adds substantial complexity and potential performance cost

Recommendation: The PR’s approach is a good incremental step: it adds a narrowly-scoped, well-tested rule (exactly two consecutive third tones) and explicitly avoids the harder 3+ run problem that needs word boundaries. For future expansion, consider moving sandhi logic onto an internal normalized tone-number model to reduce diacritic coupling while keeping the current behavior as a stable baseline.

Files changed (10) +135 / -17

Enhancement (2) +51 / -2
index.tsClarify toneSandhi scope and keep middleware placement +2/-2

Clarify toneSandhi scope and keep middleware placement

• Updates the toneSandhi option comment to include two-third-tone sandhi and adds a reference link. Renames the nearby comment to reflect broader sandhi handling (not only 一/不).

packages/pinyin-pro/lib/core/pinyin/index.ts

middlewares.tsImplement two-third-tone sandhi with lightweight third-tone detection +49/-0

Implement two-third-tone sandhi with lightweight third-tone detection

• Adds a diacritic map and a lightweight regex to detect third-tone syllables without calling the heavier tone classifier. Scans contiguous third-tone runs and, when the run length is exactly two, converts the first syllable to second tone and synchronizes originPinyin to keep downstream toneType:num consistent.

packages/pinyin-pro/lib/core/pinyin/middlewares.ts

Tests (4) +72 / -7
basic.test.jsUpdate baseline expectation for third-tone pair sandhi +1/-1

Update baseline expectation for third-tone pair sandhi

• Adjusts the expected output for “好好学习” to reflect third-tone pair sandhi on the first “好” when toneSandhi defaults to enabled.

packages/pinyin-pro/test/basic.test.js

segment.test.jsUpdate segment test expectation for third-tone sandhi output +2/-2

Update segment test expectation for third-tone sandhi output

• Updates the expected string output so “nǐhǎo” becomes “níhǎo” under tone sandhi behavior. Also normalizes file ending/newline.

packages/pinyin-pro/test/segment.test.js

special.test.jsAdjust special-case expectations impacted by third-tone pair sandhi +2/-2

Adjust special-case expectations impacted by third-tone pair sandhi

• Updates expected outputs where “不 + 你” creates an adjacent third-tone pair (e.g., “要不你走”). Updates a tongue-twister case (“好好学习”) to reflect the new rule.

packages/pinyin-pro/test/special.test.js

toneSandhi.test.jsAdd comprehensive tests for third-tone pair sandhi semantics +67/-2

Add comprehensive tests for third-tone pair sandhi semantics

• Adds coverage for: exact two-third-tone conversion, toneType outputs (symbol/num/none), array output, punctuation/non-Zh boundaries, 3+ third-tone runs unchanged, custom pinyin interaction, and preserving existing 一/不 rules. Ensures custom dict state is cleared between tests.

packages/pinyin-pro/test/toneSandhi.test.js

Documentation (4) +12 / -8
basic-params-table.vueDocument third-tone sandhi behavior in toneSandhi option (EN) +5/-3

Document third-tone sandhi behavior in toneSandhi option (EN)

• Expands the toneSandhi option description to include third-tone sandhi for two consecutive third tones. Adds examples demonstrating the effect on “你好” when enabled/disabled.

packages/docs/en/.vitepress/theme/components/basic-params-table.vue

basic-params-table.vueDocument third-tone sandhi behavior in toneSandhi option (ZH) +5/-3

Document third-tone sandhi behavior in toneSandhi option (ZH)

• Updates the Chinese docs to describe toneSandhi as covering 一/不 rules plus two-third-tone sandhi. Adds “你好” examples for both enabled and disabled modes.

packages/docs/zh/.vitepress/theme/components/basic-params-table.vue

index.tsAlign segment pipeline comment with expanded tone sandhi +1/-1

Align segment pipeline comment with expanded tone sandhi

• Updates the segment flow comment to reflect that middlewareToneSandhi now covers more than 一/不. Behavior remains that sandhi middleware runs in the segment pipeline when enabled.

packages/pinyin-pro/lib/core/segment/index.ts

index.d.tsUpdate public type docs for toneSandhi option +1/-1

Update public type docs for toneSandhi option

• Mirrors the expanded toneSandhi description in the generated d.ts to include two-third-tone sandhi and its reference link.

packages/pinyin-pro/types/core/pinyin/index.d.ts

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📎 Requirement gaps (0) 📜 Skill insights (0)

Grey Divider


Remediation recommended

1. API size tables not updated 📘 Rule violation ⚙ Maintainability
Description
This PR changes core pinyin-pro implementation but does not include regenerated API size
documentation, risking stale size tables. Per the repo’s pnpm size workflow, size tables should be
updated via tooling and committed alongside bundle-affecting changes.
Code

packages/pinyin-pro/lib/core/pinyin/middlewares.ts[R238-244]

+const thirdToneToSecondToneMap = {
+  ǎ: "á",
+  ǒ: "ó",
+  ě: "é",
+  ǐ: "í",
+  ǔ: "ú",
+  ǚ: "ǘ",
Evidence
The compliance rule requires size tables to be updated via pnpm size when changes may affect
bundle output. The PR introduces new core logic in middlewares.ts, while the repository’s size
tooling explicitly rewrites packages/pinyin-pro/README.md and
packages/docs/*/docs/guide/api-size.md, which are marked as generated by pnpm size.

CLAUDE.md: Do Not Manually Edit Generated API Size Tables; Update Them via pnpm size
packages/pinyin-pro/lib/core/pinyin/middlewares.ts[238-299]
packages/pinyin-pro/scripts/size.ts[238-269]
packages/docs/en/docs/guide/api-size.md[1-4]
packages/pinyin-pro/README.md[261-280]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PR changes library code but does not include the `pnpm size`-generated API size table updates.

## Issue Context
API size docs are explicitly marked as generated by `pnpm size`, and `packages/pinyin-pro/scripts/size.ts` overwrites the README and docs guide pages.

## Fix Focus Areas
- packages/pinyin-pro/lib/core/pinyin/middlewares.ts[238-299]
- packages/pinyin-pro/scripts/size.ts[238-269]
- packages/pinyin-pro/README.md[261-282]
- packages/docs/en/docs/guide/api-size.md[1-22]
- packages/docs/zh/docs/guide/api-size.md[1-22]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources

Grey Divider

Tip of the day
💡 Did you know, you can commit Qodo's fix in one click with committable suggestions (GitHub & GitLab)

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread packages/pinyin-pro/lib/core/pinyin/middlewares.ts
@zh-lx

zh-lx commented Aug 23, 2026

Copy link
Copy Markdown
Owner

Thanks for your PR!
Since toneSandhi is enabled by default, this change will have a significant impact on the existing pronunciation. Please give me some time to think about how to handle it better. 🤔

@luojiyin1987

Copy link
Copy Markdown
Contributor Author

Thanks for taking a look! That makes sense. Since toneSandhi is enabled by default, I agree that changing existing pronunciation behavior needs some care.

No rush — please take your time. I'm happy to adjust the PR once you decide which behavior would be the best fit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants