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: 일관된 한글 이름 규칙 설정 (to v2) #204

Merged
merged 17 commits into from
Aug 5, 2024

Conversation

Collection50
Copy link
Contributor

Overview

#191 에서 이야기를 나눈 것을 반영하여 merge 대상 브랜치를 main에서 v2로 변경합니다.

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.

okinawaa and others added 10 commits July 14, 2024 16:25
* remove extrachHangul

* Create cyan-tigers-sneeze.md

---------

Co-authored-by: Jonghyeon Ko <[email protected]>
…angul이라는 글자를 제거합니다 (toss#184)

* dissemble관련 메서드에서 hangul이름을 제거합니다

* 누락된 부분 수정

* resolve conflit

* Create late-beers-hang.md

* diassembleHangul to diassemble

---------

Co-authored-by: Jonghyeon Ko <[email protected]>
* remove acronymize

* Create weak-walls-sniff.md

---------

Co-authored-by: Jonghyeon Ko <[email protected]>
* getChoseong분리

* write test code

* getChoseong import

* remove useless import statemenet

* remove unused file
…도 함수로 분리합니다. (toss#193)

* canBe series를 별도 함수로 분리합니다

* add change set
* hasBatchim

* resolve type error

* add change set

* hasProperty, hasValueInReadOnlyStringList를 internal folder 로 옮깁니다

* resolve test error

* move with test code
* remove choseongIncludes

Co-authored-by: 서동휘 <[email protected]>
Copy link

changeset-bot bot commented Jul 28, 2024

🦋 Changeset detected

Latest commit: b231729

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

Copy link

vercel bot commented Jul 28, 2024

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

Name Status Preview Comments Updated (UTC)
es-hangul ❌ Failed (Inspect) Aug 5, 2024 5:05pm

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2024

Codecov Report

Attention: Patch coverage is 90.00000% with 4 lines in your changes missing coverage. Please review.

Please upload report for BASE (v2@01843b8). Learn more about missing BASE report.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##             v2     #204   +/-   ##
=====================================
  Coverage      ?   96.83%           
=====================================
  Files         ?       30           
  Lines         ?      601           
  Branches      ?      148           
=====================================
  Hits          ?      582           
  Misses        ?       15           
  Partials      ?        4           

@okinawaa
Copy link
Member

amountToHangul의 함수명이 amountToMoneyUnit으로 바뀐 이유가 궁금합니다!
amountToHangul이 조금 더 직관적이라고 생각이 들어서요, moneyUnit은 금액 단위만 반환해주는것아닌가? 라는 오해가 있을 수 있을 것 같아요!

@Collection50
Copy link
Contributor Author

Collection50 commented Jul 30, 2024

@okinawaa amountToHangulCurrency는 어떠신가요-??

한국 통화 느낌을 강조해보았습니다 !

amountToHangul이라는 것은 한글의 총합이라는 느낌으로 오해할 수 있을 같다고 생각해서 변경했었습니다 !

@okinawaa
Copy link
Member

okinawaa commented Aug 1, 2024

amountToHangulCurrency 좋습니다!

@Collection50
Copy link
Contributor Author

@okinawaa 수정 완료했니다 !

@okinawaa
Copy link
Member

okinawaa commented Aug 2, 2024

@manudeli 님!
amountToHangul을 amountToHangulCurrency로 변경하는것에 대해서 의견이 궁금한데 어떻게 생각하시나요?
"통화" 라는 맥락을 담는게 맞을지, 아니면 그냥 숫자를 한글로 변환만해주는다는 맥락만 담는게 좋을까요?

@okinawaa
Copy link
Member

okinawaa commented Aug 2, 2024

함수가 하는 역할은 숫자 => 한글인데, "돈" 이라는 money가 들어간 이유가 궁금해요!

@okinawaa
Copy link
Member

okinawaa commented Aug 2, 2024

우선 논의가 길어지는것 같아서 amountToHangul은 변경사항 이전으로 돌리고,
이 PR은 hangul을 없애는것만 반영하여 PR을 수정해주시는것은 어떨까요?
amountToHangul은 별도 이슈를 파서 논의하면 좋을 것 같아요!

@manudeli
Copy link
Member

manudeli commented Aug 2, 2024

@manudeli 님! amountToHangul을 amountToHangulCurrency로 변경하는것에 대해서 의견이 궁금한데 어떻게 생각하시나요? "통화" 라는 맥락을 담는게 맞을지, 아니면 그냥 숫자를 한글로 변환만해주는다는 맥락만 담는게 좋을까요?

Currency이 함수명에 포함될 필요가 없어보여요. 반드시 화폐만을 위한 기능이 아니어보여서요. 대안은 고민해보고 있습니다

@Collection50
Copy link
Contributor Author

우선 논의가 길어지는것 같아서 amountToHangul은 변경사항 이전으로 돌리고,
이 PR은 hangul을 없애는것만 반영하여 PR을 수정해주시는것은 어떨까요?
amountToHangul은 별도 이슈를 파서 논의하면 좋을 것 같아요!

네 알겠습니다 !

@okinawaa
Copy link
Member

okinawaa commented Aug 4, 2024

@Collection50 님 v2로 나아가기 위해, 이 PR의 머지가 필요해서요! 컨플릭 해결작업 진행해주시면 감사하겠습니다! 🙇

@Collection50
Copy link
Contributor Author

@okinawaa 완료했습니다 !

CHANGELOG.md Outdated
Comment on lines 3 to 21
## 1.5.0

### Minor Changes

- [#115](https://github.com/toss/es-hangul/pull/115) [`84584d4`](https://github.com/toss/es-hangul/commit/84584d48ac5ded83c55934f0b72e37a6b889f4e1) Thanks [@po4tion](https://github.com/po4tion)! - feat: 한국어를 로마자로 변환해주는 함수와 한국어를 표준 발음법으로 변환해주는 함수를 만들고 문서화를 진행합니다

## 1.4.7

### Patch Changes

- [#201](https://github.com/toss/es-hangul/pull/201) [`56db7f0`](https://github.com/toss/es-hangul/commit/56db7f0140ee369fbe0dc2dad834e8d6a218a4ea) Thanks [@BO-LIKE-CHICKEN](https://github.com/BO-LIKE-CHICKEN)! - feat: 숫자를 순 우리말 수사로 변환하거나 수 관형사로 변환하는 함수를 추가

## 1.4.6

### Patch Changes

- [#198](https://github.com/toss/es-hangul/pull/198) [`e6142b0`](https://github.com/toss/es-hangul/commit/e6142b04159133dbcab6f2771baa88adf7aa4a45) Thanks [@linenumbertwo](https://github.com/linenumbertwo)! - feat: "이라/라" 케이스 추가

- [#203](https://github.com/toss/es-hangul/pull/203) [`fbe3ad6`](https://github.com/toss/es-hangul/commit/fbe3ad67f4bd796773f60f0ab04359135b03d414) Thanks [@kangju2000](https://github.com/kangju2000)! - fix: eslint CI가 제대로 작동하지 않는 문제를 수정합니다.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 체인지 로그는 삭제된 이유가 있을까요?!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

잘못 삭제된 것으로 보입니다 ! 수정해두겠습니다 !

src/index.ts Outdated
Comment on lines 16 to 37
export {
assemble,
combineCharacter,
combineVowels,
curriedCombineCharacter,
convertQwertyToHangul,
convertQwertyToAlphabet,
disassemble,
disassembleToGroups,
disassembleCompleteCharacter,
josa,
removeLastCharacter,
hasBatchim,
canBeChoseong,
canBeJongseong,
canBeJungseong,
getChoseong,
amountToHangul,
romanize,
susa,
standardizePronunciation,
};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

export 구문으로 바로 import 후 export 를 시키고 있었는데,
이렇게 import / export 따로 진행하신 이유가 있나요?
두번씩 작성해야하는것에 대비하여 장점을 크게 못느껴서요!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

수정해두겠습니다 !!

src/index.ts Outdated
export { canBeChoseong, canBeJongseong, canBeJungseong } from './canBe';
export { getChoseong } from './getChoseong';
import { assemble } from './assemble';
import { combineCharacter, combineVowels, curriedCombineCharacter } from './combineCharacter';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curriedCombineCharacter는 외부로 노출하지 않는것이 좋을 것 같아요
combineCharacter 로 충분히 구현가능하고 combineCharacter 함수가 매우 작은 단위이므로 요구사항을 충분히 사용하는쪽에서 구현가능하다고 생각해요!!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

네 알겠습니다 !

Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

너무 감사합니다!!

@okinawaa okinawaa merged commit 82e03c3 into toss:v2 Aug 5, 2024
9 of 10 checks passed
@github-actions github-actions bot mentioned this pull request Aug 5, 2024
Merged
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.

4 participants