feat: 숫자를 한글로 바꾸는 함수 추가 #254
Conversation
🦋 Changeset detectedLatest commit: 9654c8f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #254 +/- ##
==========================================
- Coverage 99.66% 99.53% -0.14%
==========================================
Files 36 38 +2
Lines 601 639 +38
Branches 145 153 +8
==========================================
+ Hits 599 636 +37
- Misses 2 3 +1 |
| HANGUL_NUMBERS, | ||
| HANGUL_DIGITS, | ||
| HANGUL_CARDINAL, | ||
| } from '@/_internal/constants'; | ||
|
|
||
| export function amountToHangul(amount: string | number) { |
There was a problem hiding this comment.
amountToHangul은 다음 메이저 버전 업데이트 때 제거 될 것 같은데,
주석으로 deprecated를 명시해두는것은 어떨까요?
There was a problem hiding this comment.
네 저도 이 부분 함께 논의해보고 싶었습니다.
docs: deprecated amountToHangul에 반영했어요!
| 다양한 요구사항에 대응하도록 '만(萬)' 단위로 띄어쓰기를 지원합니다. | ||
|
|
||
| ```typescript | ||
| function numberToHangulMixed(input: number, options?: { spacing?: boolean }): string; |
There was a problem hiding this comment.
함수명의 맥락을 고려할 때, numberToHangulMixed는 “숫자를 한글로 변환하고, 그 결과가 섞인 형태로 나타난다”는 순서와 흐름을 더 잘 설명하는 반면, numberToMixedHangul은 “숫자와 한글이 섞인 형태로 변환한다”는 점을 먼저 강조하고 �있는 것 같은데요.
고민을 해봤는데, 숫자를 한글로 변환하는게 메인 기능이므로 진행해주신 numberToHangulMixed 가 더 적절해보이네요 👍
There was a problem hiding this comment.
네 저도 함수명에 고민이 많았는데 공감대가 형성되어 기쁘네요!
이외의 명명 기준은 다음과 같습니다.
numberToHangul이 기본값이 될 것이라고 생각했습니다.numberToHangul와 같은 인터페이스로 추가되는 함수들은numberToHangul와 가장 닮은 이름을 가지기를 바랐어요.
| 다양한 요구사항에 대응하도록 '만(萬)' 단위로 띄어쓰기를 지원합니다. | ||
|
|
||
| ```typescript | ||
| function numberToHangul(input: number, options?: { spacing?: boolean }): string; |
There was a problem hiding this comment.
input에 '133314' 처럼 string이 들어오지 않기로 하신 이유가 있나요? 저도 number만 받는게 좋을 것 같다고 생각해요!
- 내부에서 string value를 처리하는데 들어가는 복잡성
- 해당 함수는
숫자를 한글 단위로 변환하는 것이 핵심이라서, 단일 책임만 가졌으면 하기 때문입니다
There was a problem hiding this comment.
네 저도 동일한 의견입니다.
es-hangul에서 제공하는 api들은 최소한의 기능과 책임을 가졌으면 좋겠어요.
numberToHangul의 경우에도 string이 들어가면서 복잡해지는 것보다는
numberToHangul을 사용하는 곳에서 형변환해서 넣어주는 것이 어렵지도 않고 함수가 가벼워질 것이라고 생각했습니다.
앞으로도 api를 제공할 때 사용하는 곳에서 유용하게 사용할 수 있는가? 도 함수의 책임과 함께 고민되면 좋을 것 같습니다!
|
@okinawaa 자리 수 관련 수정은 docs: fix numberToHangulMixed examples에 반영했습니다 🙏🏻 |
Co-authored-by: 박찬혁 <[email protected]>
Overview
#214 에서 논의되었던 숫자를 한글로 바꿔주는 함수를 추가합니다
왜 추가하나요?
기존의 amountHangul은 물론 좋은 기능을 제공해 주는 함수였지만, 함수의 책임이 명확하지 않았고 다양한 입력에 열려있고 그 반환값을 사용할 수 있는 곳이 적었습니다.
그래서 꼭 필요한 기능만 구현하고 앞에 "일"을 붙이거나 뒤에 "원정"등을 붙이는 부분은 사용하는 곳에 위임한 함수를 추가합니다
numberToHangul과numberToHangulMixed추가함께 논의 하고 싶은 것
https://github.com/huskyhoochu/num-to-korean 을 이슈에서도 다루고 실제 구현시에도 참고를 했는데
병합이 된다면 기존의 slash 같은 라이브러리에서는 이런 것들을 어떻게 다루셨는지 궁금합니다.
PR Checklist