Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

sa02045
Copy link
Contributor

@sa02045 sa02045 commented Jul 9, 2024

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

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

@sa02045 sa02045 requested review from okinawaa and raon0211 as code owners July 9, 2024 13:49
Copy link

changeset-bot bot commented Jul 9, 2024

⚠️ No Changeset found

Latest commit: 0c087db

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

Copy link

vercel bot commented Jul 9, 2024

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
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files

Impacted file tree graph

@@            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
Copy link
Member

감사합니다!

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

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

@okinawaa
Copy link
Member

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

@sa02045
Copy link
Contributor Author

sa02045 commented Jul 14, 2024

감사합니다!

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

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

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

@sa02045
Copy link
Contributor Author

sa02045 commented Jul 14, 2024

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

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

@sa02045 sa02045 closed this Jul 14, 2024
@okinawaa
Copy link
Member

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

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.

3 participants