Skip to content

Fix the order of the spacing rules for brackets and symbols in Joiner - #278

Merged
bab2min merged 2 commits into
bab2min:mainfrom
heeoneie:fix/issueP220-joiner-space
Oct 15, 2026
Merged

Fix the order of the spacing rules for brackets and symbols in Joiner#278
bab2min merged 2 commits into
bab2min:mainfrom
heeoneie:fix/issueP220-joiner-space

Conversation

@heeoneie

@heeoneie heeoneie commented Sep 22, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown

bab2min/kiwipiepy#220에서 보고된 join의 띄어쓰기 문제 중 두 가지를 고칩니다.

재현

0.23.2에서도 재현됩니다. 다만 join에 Token 객체를 그대로 넘기면 원문의 위치 정보로
띄어쓰기가 복원되기 때문에, (형태, 품사) 튜플로 넘길 때만 드러납니다.

>>> text = '1인 이상. 9%씩. (mike)'
>>> kiwi.join(kiwi.tokenize(text))                              # 위치 정보 있음
'1인 이상. 9%씩. (mike)'
>>> kiwi.join([(t.form, t.tag) for t in kiwi.tokenize(text)])   # 품사만으로 판단
'1 인 이상. 9 %씩. ( mike)'

원인

isSpaceInsertable의 규칙 두 쌍이 서로 모순돼 있었습니다.

1. 여는 괄호 뒤

l == sso || l == ssc 예외가 "SL이나 SN 앞에는 공백을 넣는다" 규칙보다 아래에 있어서,
괄호 뒤에 오는 것이 영문·숫자일 때만 공백이 들어갔습니다.

입력 결과
( + mike(SL) ( mike
( + 1(SN) ( 1
( + 가(NNG) (가

괄호 예외를 다른 좌측 예외들(xpn, so, ss, sw)과 같은 자리로 옮겨, 뒤에 무엇이
오든 동일하게 동작하도록 했습니다.

2. 숫자 뒤 기호

SW 앞에는 공백을 넣으면서 SW 뒤에는 넣지 않았습니다(l == sw가 이미 false를
반환). 그래서 SN SW SN은 3 +4가 되는데, 이는 3 + 4도 3+4도 아닙니다.
국어 표기에서 숫자에 뒤따르는 기호는 붙여 쓰므로(9%, 30℃, 100$), 기존
SN + NNB 예외 옆에 SN + SW를 추가했습니다.

고치지 않은 것

같은 보고에 있는 1 + 인 → 1 인은 Joiner 규칙의 문제가 아니라고 보아 그대로
두었습니다. 숫자 뒤 단위명사는 NNB로 태깅되면 이미 붙습니다.

붙는 경우(NNB) 안 붙는 경우(NNG)
3등, 10위, 5번, 3명, 2년, 4월, 1시, 6개 1 인, 5 층, 2 팀, 7 급, 3 배, 1 종

즉 차이는 Joiner가 아니라 해당 형태를 NNB로 보느냐 NNG로 보느냐에서 생깁니다.
또한 별도 모듈인 Kiwi::space()도 1인이상 → 1 인 이상을 내놓아 두 모듈의 동작이
일치하고 있어, 한쪽만 바꾸면 오히려 불일치가 생깁니다. isSpaceInsertable은
GenerativeMADataset의 학습 데이터 생성에도 쓰이므로 이 부분은 임의로 건드리지
않았습니다. 관련 조사 내용은 이슈 쪽에 코멘트로 남겨두겠습니다.

테스트

  • TEST(KiwiCpp, IssueP220)을 추가했습니다. 보고된 두 조합, 공백이 유지되어야 하는
    경우(단어 뒤의 여는 괄호, 한글 뒤의 기호), 그리고 문장을 analyze → Joiner로
    다시 이어 원문이 복원되는지를 함께 확인합니다.
  • 앞 커밋(Fix the order ...) 없이 이 테스트는 8개 단언에서 실패합니다.
  • macOS(arm64, AppleClang) Release 빌드에서 kiwi-test 전체를 돌렸습니다.
    수정 전 163개 통과 → 수정 후 164개 전부 통과.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes

    • Improved spacing when joining analyzed Korean text.
    • Prevented unwanted spaces after parentheses and between numbers and following symbols, such as 9% and 30℃.
    • Preserved expected spaces before opening parentheses and after Hangul text.

  • Tests

    • Added coverage confirming corrected spacing and accurate reconstruction of complete text.

heeoneie and others added 2 commits September 22, 2026 14:59
@heeoneie @claude
Fix the order of the spacing rules for brackets and symbols in Joiner
a075553 
`isSpaceInsertable` tested `l == sso || l == ssc` only after the rule that
inserts a space in front of every SL or SN, so an opening bracket kept a
space when the next morpheme was alphabetic or numeric.  `(` + `mike` was
joined as `( mike)` while `(` + `가` was already joined as `(가`.  The
bracket exception now sits with the other left-hand exceptions, above that
rule, so it no longer depends on what follows the bracket.

The symbol rules were asymmetric in the same way.  A space was inserted in
front of SW but never after it, because `l == sw` already returned false,
so `SN SW SN` was joined as `3 +4`, which is neither `3 + 4` nor `3+4`.  A
symbol that follows a number is attached to it in Korean writing (`9%`,
`30℃`, `100$`), so SN followed by SW is now joined without a space, next to
the existing `SN` + `NNB` exception.

Both cases come from bab2min/kiwipiepy#220.  The remaining case in that
report, `1` + `인` joined as `1 인`, is not a Joiner rule problem: numeral
units tagged NNB (`3등`, `10위`, `3명`) are already attached, and only the
ones tagged NNG (`인`, `층`, `팀`, `급`) are not, so it is left alone here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@heeoneie @claude
Add test cases for the spacing rules of Joiner
20e87ca 
The test covers the two combinations that were reported broken, the cases
that must keep their space (a bracket that follows a word, a symbol that
follows a Hangul noun), and a round trip of whole sentences through
`analyze` and `Joiner`, which is how the report was written.

Without the previous commit the test fails on eight assertions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Sep 22, 2026 •
edited
Loading

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 3595a7bd-7780-428a-b257-94ea39dde094

📥 Commits

Reviewing files that changed from the base of the PR and between 280302a and 20e87ca.

📒 Files selected for processing (2)
  • src/Joiner.hpp
  • test/test_cpp.cpp

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

📝 Walkthrough

Walkthrough

The joiner consolidates punctuation checks and prevents spaces between numeric tags and sw or nnb. A new test covers punctuation, symbol spacing, and reconstruction of analyzed Korean text.

Changes

Joiner spacing correction

Layer / File(s) Summary
Spacing rules and regression coverage
src/Joiner.hpp, test/test_cpp.cpp		 isSpaceInsertable now returns false for sn followed by sw or nnb. The test verifies spacing around parentheses and symbols and confirms round-trip text reconstruction.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: bab2min

Merge Risk: ⚪ Minimal · up to 20e87

The joiner now keeps numeric-symbol pairs attached while preserving bracket and Hangul spacing, with focused round-trip coverage and no merge-blocking risk identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 4 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: correcting bracket and symbol spacing rules in Joiner. It is concise, specific, and consistent with the implementation and tests.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@heeoneie heeoneie mentioned this pull request Sep 22, 2026
Open
@bab2min

bab2min commented Oct 15, 2026

Copy link
Copy Markdown
Owner

@heeoneie 좋은 기여 해주심에 감사드립니다.
형태소 재결합 시 띄어쓰기 규칙을 교정해주신 것 살펴보니 처음 규칙에 누락된 부분이 있던것이 맞았습니다. 수정 내용도 문제 없고 모든 테스트를 통과했으니 머지하도록 하겠습니다!

@bab2min
bab2min merged commit 48df671 into bab2min:main Oct 15, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@heeoneie @bab2min