toss / es-hangul

A modern JavaScript library for handling Hangul characters.
https://es-hangul.slash.page/
MIT License
1.24k stars 83 forks source link

fix: hangulIncludes 올바르지 않은 동작 #178

Closed sa02045 closed 1 month ago

sa02045 commented 2 months ago

Overview

106 를 닫습니다.

hangulIncludes 함수의 의도에 맞는 결과값을 리턴합니다.

AS-IS

hangulIncludes('123', '12'); // true 
hangulIncludes('abc', 'ab'); // true
hangulIncludes('你好', '你'); // true
hangulIncludes(['12', '123', '123123'], ['12']); // true
hangulIncludes('{"a": "가나다라"}', '가나다라'); // true

TO-BE

hangulIncludes('123', '12'); // false 
hangulIncludes('abc', 'ab'); // false
hangulIncludes('你好', '你'); // false
hangulIncludes(['12', '123', '123123'], ['12']); // false
hangulIncludes('{"a": "가나다라"}', '가나다라'); // false

Description

"문자열"과 "한글 문자열"을 구분할 필요가 있습니다.

만약 "한글 문자열"을 나타내는 타입 Hangul이라는 타입이 존재한다면 함수의 인자 타입은 다음과 같이 나타내볼 수 있습니다.

// 문자열
hangulIncludes(x: string, y: string)

// 한글 문자열
disassembleHangul(str: Hangul)

safeParseHangul 함수를 사용하여 "문자열"을 "한글 문자열"으로 좁힙니다. disassembleHangul와 같이 "한글 문자열"을 처리하는 내부함수의 올바른 동작을 보장합니다.

PR Checklist

  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.
changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: 0c087db54f5f1621b8859de21b18eb209c3b53f9

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
es-hangul ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 9, 2024 1:50pm
codecov-commenter commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 100.00%. Comparing base (587fb04) to head (0c087db).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/toss/es-hangul/pull/178/graphs/tree.svg?width=650&height=150&src=pr&token=My9jTW6bSr&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss)](https://app.codecov.io/gh/toss/es-hangul/pull/178?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) ```diff @@ Coverage Diff @@ ## main #178 +/- ## ========================================= Coverage 100.00% 100.00% ========================================= Files 16 16 Lines 260 264 +4 Branches 58 59 +1 ========================================= + Hits 260 264 +4 ```
okinawaa commented 1 month ago

감사합니다!

hangulIncludes('{"a": "가나다라"}', '가나다라'); // false

첫번째 인자에 두번째 인자인 가나다라 가 포함되있음에도 불구하고, false를 반환하도록 구현하신 이유가 궁금합니다!

okinawaa commented 1 month ago

v2에서 #176 에서 논의한 결과에따라서 hangulIncludes는 사용자가 원자적인 es-hangul의 함수 disassemble을 활용해 사용자의 비즈니스 요구사항을 만족시킬 수 있도록 자유도를 열어주면서 hangulIncludes를 삭제하려고 해요. @sa02045 님께서도 동의하신다면 이 PR은 닫아도 될 것 같아요!

sa02045 commented 1 month ago

감사합니다!

hangulIncludes('{"a": "가나다라"}', '가나다라'); // false

첫번째 인자에 두번째 인자인 가나다라 가 포함되있음에도 불구하고, false를 반환하도록 구현하신 이유가 궁금합니다!

hangulIncludes의 인자가 모두 한글문자열이어야 한다고 생각했습니다! 두 인자가 하나라도 한글이 아닌 문자열인 경우 false를 반환하도록 구현했습니다.

sa02045 commented 1 month ago

v2에서 #176 에서 논의한 결과에따라서 hangulIncludes는 사용자가 원자적인 es-hangul의 함수 disassemble을 활용해 사용자의 비즈니스 요구사항을 만족시킬 수 있도록 자유도를 열어주면서 hangulIncludes를 삭제하려고 해요. @sa02045 님께서도 동의하신다면 이 PR은 닫아도 될 것 같아요!

넵. 프로젝트의 방향성에 공감합니다! 이 PR은 close할게요

okinawaa commented 1 month ago

상세한 이슈레이징과, PR을 통한 구현 감사드립니당 !! 🙇