-
Notifications
You must be signed in to change notification settings - Fork 79
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
feat: 두 문자열의 유사도를 측정할 수 있는 getSimilarity 함수를 만들고, 문서화를 진행합니다. #104
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: 9661ec4 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 |
Screen.Recording.2024-05-26.at.2.07.17.AM.mov |
src/calculateSimilarity.ts
Outdated
@@ -0,0 +1,37 @@ | |||
import { disassembleHangul } from './disassemble'; | |||
|
|||
export function calculateSimilarity(a: string, b: string): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이름이 더 심플하면 좋을 것 같아요. calculate가 없어도 좋을 것 같더라구요. date-fns가 add, format과 같은 이름으로 단순하게 제공하는 것 처럼요
export function calculateSimilarity(a: string, b: string): number { | |
export function similarity(a: string, b: string): number { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오오 이거 넘 좋은 관점이네요,
한번 고민 후 다시 코맨트 남기겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음 제공할때 이름을 확실히 정하고 나가는게 좋을 것 같아서 깊게 고민해보죠!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
저는 계산한다는 맥락이 굳이 외부에 노출될 필요가 있는 맥락인가하는 생각이 있어서, getSimilarity
라는 이름도 충분하다고 생각합니다.
@manudeli 님께서 제안주신 similarity
는 명사라, const similarity = 유사도함수()
와 같은 use case에서 함수의 결과가 담길 변수와 이름이 충돌할 수 있다는 생각도 들어요!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSimilarity 좋습니다! 간결하고 오해의 소지가 없을 것 같아요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@okinawaa @evan-moon 함수들 명명 규칙을 정해보면 좋겠어요. BREAKING CHANGE가 있기는 하겠지만 아직 사용량이 적을 때 명명규칙을 정하고 빠르게 BREAKING CHANGE를 내는 것도 장기적으로 도움이 될 수 있을 거 같아요
disassembleHangul, disassembleHangulToGroup도 disassemble로는 어떨까요? (BREAKING CHANGE 필요) Hangul이라는 맥락을 알필요가 있었다면 getSimilarityHangul이어야 하는지 아닌지 정의할 필요가 있을 거 같아요
disassemble(korStr, { type: 'string' }) // disassembleHangul
disassemble(korStr, { type: 'array' }) // disassembleHangulToGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 제안인 것 같습니다.
저는 가급적이면 Hangul이라는 맥락이 함수명에 포함되는 것이 좋다고 생각하는데요.
물론 import { disassemble } from 'es-hangul'
과 같은 구문을 보면 "이 함수가 한글을 분해한다"라는 맥락을 파악할 수는 있겠으나, import 문의 특성 상 모듈의 최상단에 위치하여 함수가 소비되는 곳과의 거리가 멀 가능성이 높다는 점이 그 이유인데요.
이런 상황이라면 개발자가 disassemble
이라는 추상 레벨이 높은 함수명만 보고 함수의 역할과 동작을 예측하기 어렵기 때문에 좋은 네이밍이 아니라고 생각했어요.
그래서 함수명만으로도 명확하게 함수의 역할과 동작을 예측할 수 있는 *Hangul
이라는 네이밍이 더 DX에 좋지 않을까 했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
물론 import { disassemble } from 'es-hangul'과 같은 구문을 보면 "이 함수가 한글을 분해한다"라는 맥락을 파악할 수는 있겠으나, import 문의 특성 상 모듈의 최상단에 위치하여 함수가 소비되는 곳과의 거리가 멀 가능성이 높다는 점이 그 이유인데요.
저도 이 점에 동의해요. 그리고 이에 더해서 date-fns와는 es-hangul이 인자를 받는 데에 다른 특징을 갖고 있다고 생각했어요. date-fns의 add, format는 date라는 맥락이 함수명에 포함되지 않아도 괜찮은 게 인자로 받는 변수명에 그 내용이 포함될 가능성이 높아서라고 생각했어요.
add(createdAt, { day: 1 }) // 변수명 createdAt에서 Date객체임을 알 수 있음
add('hello', { day: 1 }) // 타입에러! Date객체여야 함
es-hangul의 경우 변수명이나 타입적으로 인자가 변수라는 것을 알 수 없기 때문에 Hangul이라는 맥락이 disassembleHangul에 포함되는 것이 더 좋다고 생각했어요
Bad
disassemble(user.name) // user.name이 한국어야 한다는 것을 알 수 없음
disassemble('English Name') // 한국어가 아니더라도 타입에러가 발생하지 않음
Good
disassembleHangul(user.name) // user.name이 한국어야 한다는 것을 알 수 없음. 하지만 hangul인지 검사 필요하다는 것을 이해할 수 있음
disassembleHangul('English Name') // 한국어가 아니더라도 타입에러가 발생하지 않음. 하지만 hangul이 들어와야할 곳에 영어가 들어온 것이 체크해야한다는 점을 이해할 수 있음
그래서 String이지만 한글이어야하는 인자를 받는 경우에 함수명에 ~~~Hangul
과 같은 suffix가 있으면 하는 바람이 있습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이슈화 해두었습니다 #121
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM입니다
커밋메시지에도 getSimilarity로 나오도록 PR title와 description에도 calculateSimilarity -> getSimilarity를 반영하면 더 좋을 거 같아요 |
function getSimilarity( | ||
// 비교할 첫 번째 한글 문자열 | ||
left: string, | ||
// 비교할 두 번째 한글 문자열 | ||
right: string | ||
): number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
엄청 중요하진 않은 것 같은데, 문서에서는 left, right 라는 표현을 썼는데 주석에는 첫 번째 / 두 번째 라고 쓰이고 테스트 코드에서는 target, input 이라는 표현이 쓰이고 있어요. 통일된 표현을 사용하면 어떨까요? 🤔
|
||
# getSimilarity | ||
|
||
두 한글 문자열의 유사도를 계산하여 0에서 1 사이의 값을 반환합니다. 이 함수는 es-hangul 라이브러리의 disassemble 메서드를 사용하여 한글 문자열을 자모 단위로 분해하고, Levenshtein 거리 알고리즘을 이용해 문자열 간의 최소 편집 거리를 계산한 후 이를 바탕으로 유사도를 계산합니다. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
한글이라기보다는 두 문자열에 대한 유사도 측정인데, 문서에서 한글이라고 표현하는 것이 좋을까요?
혹은 영문이 섞이면, 의 엣지 케이스 등을 테스트 코드에서 대응해야 할까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음.. 이 부분 너무 잘 말씀해주신 것 같네요..
몇 가지 방법이 생각나는데 어느 방안이 가장 베스트일지 고민입니다!! ㅠㅠ
- 한글 이외의 값이 left, right에 들어있으면 에러를 던진다.
- 한글 이외의 값이 left, right에 들어있으면 무조건 0을 리턴한다.
- 한글 이외의 값이 left, right에 들어있으면 한글 제외 문자를 제거하고 값을 계산한다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내부에서 복잡한 처리를 하기 보다는,
특수문자와, 영어, 숫자를 받을 수 있도록 처리해줬습니다!
- 문서에 한글 문자열이라는 사항을 문자열 로 변경했습니다.
- 적절한 테스트코드를 작성해줬습니다!
좋은 코맨트 감사합니다!
Co-authored-by: Jonghyeon Ko <[email protected]>
Co-authored-by: Jonghyeon Ko <[email protected]>
Co-authored-by: Jonghyeon Ko <[email protected]>
Co-authored-by: Jonghyeon Ko <[email protected]>
Co-authored-by: Jonghyeon Ko <[email protected]>
Co-authored-by: Jonghyeon Ko <[email protected]>
너무나 많은 분들이 도움을 주셨지만, 이 메서드의 코어한 로직은, 한글의 복잡도를 이해한 뒤, 유사도를 계산한다기 보다는 Levenshtein 을 사용하는것에 불과하다고 생각합니다. 이 기능을 구현하고 싶다면, disassembleHangul 함수와 levenshetein 알고리즘을 이용해서 직접 구현할 수 있을거라고 생각합니다. |
fix #49
Overview
두 문자열의 유사도를 측정할 수 있는 getSimilarity 함수를 만들고, 문서화를 진행합니다.
두 한글 문자열의 유사도를 계산하여 0에서 100 사이의 값을 반환합니다. 이 함수는 es-hangul 라이브러리의 disassembleHangul 메서드를 사용하여 한글 문자열을 자모 단위로 분해하고, Levenshtein 거리 알고리즘을 이용해 문자열 간의 최소 편집 거리를 계산한 후 이를 바탕으로 유사도를 계산합니다.
PR Checklist