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

feat: 숫자를 순 우리말 수사로 변환하거나 수 관형사로 변환하는 함수를 추가 #201

Closed BO-LIKE-CHICKEN closed 1 month ago

BO-LIKE-CHICKEN commented 1 month ago

Overview

169 에서 제안드린 함수를 추가합니다.

숫자를 순 우리말 수사로 변환하거나 수 관형사로 변환합니다. 주어진 숫자가 0보다 크고 100 이하일 때 유효합니다.

PR Checklist

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

🦋 Changeset detected

Latest commit: 3d35964496de3b6b8bcb68aa983fcc06680e2c71

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

vercel[bot] commented 1 month 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 Aug 1, 2024 1:59am
codecov-commenter commented 1 month ago

Codecov Report

Attention: Patch coverage is 96.77419% with 1 line in your changes missing coverage. Please review.

Project coverage is 95.34%. Comparing base (506d0fc) to head (5e3ea59).

Additional details and impacted files [![Impacted file tree graph](https://app.codecov.io/gh/toss/es-hangul/pull/201/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/201?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=toss) ```diff @@ Coverage Diff @@ ## main #201 +/- ## ========================================== + Coverage 95.18% 95.34% +0.15% ========================================== Files 17 18 +1 Lines 291 322 +31 Branches 67 77 +10 ========================================== + Hits 277 307 +30 - Misses 13 14 +1 Partials 1 1 ```
BO-LIKE-CHICKEN commented 1 month ago

feat: 정수가 아닌 경우에 대한 예외처리를 추가하고 분기를 탈 확률이 가장 낮은 조건을 뒤로 배치

test: 정수가 아닌 값에 대한 테스트케이스 추가

에 정수가 아닌 값에 대한 예외처리도 추가하여 의견 주신 내용을 반영했습니다. 🙏🏻

minsoo-web commented 2 weeks ago

안녕하세요, 이 부분에 대해서 제안 드려보고 싶은 점과 의문인 점이 있었어요

100 이상은 왜 안 되지?

우선 docs 를 제대로 안 읽은 사람은

image

스크린샷의 정보만 읽을 것 같아서 JSDoc 이 추가되면 좋을 것 같구요 (타입으로 100 이하를 강제할 수는 없으니)

99 -> 아흔아홉 101 -> 백하나

가 같은 맥락이라고 생각이 들었어요. 제가 잘 모르는 한글 도메인 맥락의 내용이 있다면 깨우쳐주시면 감사하겠습니다

확장가능의 여부라기 보다 인터페이스에서 상상이 안 되는 결과 값이에요

저도 그 고민을 해보았지만, 확장할 여지가 없는 코어(?)에 가까운 함수라는 생각을 했어요. 제가 고민했던 확장 기능들은 다음과 같습니다.

라고 답변을 해주셨는데 저는 아래와 같은 인터페이스를 봤을 때

susa(1, true) // ? false 주면 안 바꿔주나?

수사 1 트루 라고 읽거든요, 어떤 결과 값이 나올지 doc 을 무조건 봐야 하는 인터페이스가 개선되었으면 하는데 어떻게 생각하시는지 궁금합니다.

BO-LIKE-CHICKEN commented 2 weeks ago

안녕하세요 @minsoo-web 님. 좋은 고민거리를 함께 나눠주셔서 감사해요!

먼저, 제 생각을 말씀드리기전에 의문이 드시는 부분을 다시금 정리해 보았어요. 의도를 잘못 파악했다면 말씀부탁드려요 🙏🏻

  1. 100 이상은 susa에서 왜 지원하지 않을까?
  2. susa(1,{classifier: false})보다 susa(1, true)의 형식의 인터페이스가 나은 이유가 뭘까? 기존의 답변도 쉽게 수긍되지는 않는다.
  3. docs를 읽어야만 이해가 되는 부분들이 많은데 왜 JSDoc으로 제공하지 않고 있을까? 혹은 개선할 방법이 없을까?

100 이상은 susa에서 왜 지원하지 않을까?

이 부분에서는 저도 구현 당시에 오류가 있었던 것 같아요.

정확하게는 100보다 작은 경우에만 susa에서 지원되어야 합니다.

susa는 문서에 남겨둔 것처럼 순 우리말 수사로 변환하거나 수 관형사로 변환해 주는 함수에요. 즉, 백, 천, 만등의 한자어 표현은 지원하지 않아야 해요. 따라서 백을 초과하는 경우에는 순 우리말 수사로 표현하는 것이 적절하지 않다고 생각했어요.

백을 지칭하는 "온"등의 표현도 있지만 아무래도 대부분의 사람들이 익숙하지 않기 때문에 이를 지원했을시에 더욱 혼동을 줄 수 있다는 생각이 들었어요. 그래서 100 이상의 경우에는 지원을 안하는 것이 좋으며, 100 이상의 경우에는 사용하는 곳에 그 책임을 위임해야한다는 의견입니다.

(또한 후술하겠지만, docs를 읽지않으면 지원 범위를 파악하기 힘들다는 점은 공감해요.)


susa(1,{classifier: false})보다 susa(1, true)의 형식의 인터페이스가 나은 이유가 뭘까? 기존의 답변도 쉽게 수긍되지는 않는다.

저는 두 번째 인자로 객체를 받는 것의 가장 큰 목표는 확장에 열려있는 것이라고 생각했어요.

기존의 질문

요거 인터페이스를 아래와같이 해서 확장성을 가져갈 수도 있을 것 같은데, boolean을 그대로 받도록 하신 이유가 있으신가요? 저는 둘 다 좋은데, 이 기회에 es-hangul에 컨벤션(?) 을 잡아갈수도 있을 것 같아서요!

options : {
  classifier?:boolean
}

하여, 확장 가능성에 대하여 답변을 드렸었어요.

물론 susa(1, { classifier: false })susa(1, true)보다 예상이 가능한 인터페이스인가? 라고 한다면 Yes입니다. 하지만, 컨벤션화 되는 부분이 아니라면 다음과 같은 이유로 저는 여전히 지금의 인터페이스를 유지하기를 희망합니다.

1. 확장될 여지가 없는데 객체로 인자를 전달받아야 하는가?

반대로 docs를 읽고 사용하는 상황이라면 되려 boolean을 전달해주는 것이 단순하고 간결하게 사용할 수 있을 것이라고 생각해요.

2. susa(1, { classifier: false })가 조금 더 예측가능한 인터페이스지만 classifier가 true일때 어떤 값을 제공해주는지 문서를 보지 않고도 이해할 수 있을까?

결국에는 classfiertrue일때 어떤 반환값을 제공하는지 보기 위해서는 JSDoc이나 문서를 보게될 것 같아요.

3. 이미 api가 제공되고 있는 시점에서 바꾸는 것이 옳을까?

이미 api가 제공되고 있는 시점에서 Breaking Change를 하는 것은 조금 고민 됩니다. 당장은 JSDoc이 추가된다면 어느정도 해소될 것이라고 생각해요.

어떻게 생각하시는지 의견을 듣고 싶습니다. 🙇🏻‍♂️


docs를 읽어야만 이해가 되는 부분들이 많은데 왜 JSDoc으로 제공하지 않고 있을까? 혹은 개선할 방법이 없을까?

아래와 같이 이전 PR에서 남겼던 적이 있어요.

컨벤션(?) 이야기가 나와서 코드를 살펴보다보니 규칙을 정하면 좋을 법한 부분들이 많은 것 같아요.

문자열을 받는 파라미터의 명칭이 어떤 곳에서는 str 다른 곳에서는 word, words(배열이 아님에도)으로 쓰이고 있다. jsdoc 형태로 함수를 설명해 주는 곳이 있고, .md에서만 함수에 대한 설명을 볼 수 있는 곳이 있다. 등이 기여 시점에 기여자에게 안내되어 더 통일성있는 코드가 추가되면 좋겠네요. 🙂

하여, 이 부분에 대한 컨벤션이나 가이드가 정해지면 추가해둘 생각을 하고 있었어요. (당시에는 josa 함수의 인터페이스와 문서 구조등을 따라가고자 노력했거든요)

하지만 이미 JSDoc 형태로 제공하는 곳들도 많으니 당장에 추가가 되어도 괜찮겠다는 생각이 드네요! 아울러 susa 뿐만 아니라 es-hangul에서 JSDoc이 제공되지 않는 부분들이 채워지면 무척 좋겠어요.

긴 글 읽어주셔서 감사합니다 🙏🏻