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.

Sign up for GitHub

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

docs: CONTRIBUTING.md 파일에 메서드 컨벤션을 작성합니다. #132

Merged
merged 7 commits into from
Jul 3, 2024
Merged

docs: CONTRIBUTING.md 파일에 메서드 컨벤션을 작성합니다. #132

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e08c928
Update CONTRIBUTING.md
okinawaa Jun 25, 2024
5f563be
fix
okinawaa Jun 30, 2024
830e915
write contribution.md
okinawaa Jul 2, 2024
43c46c5
Merge branch 'main' into okinawaa-patch-3
okinawaa Jul 2, 2024
968ba68
Update .github/CONTRIBUTING.md
okinawaa Jul 3, 2024
f44d801
Update .github/CONTRIBUTING.md
okinawaa Jul 3, 2024
38dc523
Update .github/CONTRIBUTING.md
okinawaa Jul 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .github/CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,3 +48,22 @@
### 2.2 Description

A clear and concise description of what the pr is about.

## 3. Convention

es-hangul에서 제공하는 메서드들은 특별한 이유가 없다면, hangul이라는 맥락을 드러내는 것을 지향합니다.

```ts
// Don't
function getSimilarity()
// Do
function getHangulSimilarity()
Copy link
Member

Choose a reason for hiding this comment

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

이전에 동사형이면 좋겠다는 것은 알지만 hangul이 맥락을 넣는다면 아래와 같이 간결하게 표현하는 것은 어떨까요?

Suggested change
function getHangulSimilarity()
function similarityHangul()

Copy link
Member Author

Choose a reason for hiding this comment

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

similarityHangul 은 메서드로 보일지 의문이에요
아마 아래와 같이 사용될텐데 어색하지 않을까요 😢 ?

const hangulSimilarity = similarityHangul(x,y);

Copy link
Member

@manudeli manudeli Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

다른 주제이긴한데 Hangul의 위치가 맨 뒤로 고정되면 좋겠어요

getSimilarityHangul(text1, text2)

Copy link
Collaborator

@po4tion po4tion Jun 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

getHangulSimilarity가 영문법 해석상으로는 좀 더 매끄럽게 한글로 변환되기는 합니다.

  1. getHangulSimilarity = get Hangul Similarity = 한글 유사성을 얻다
  2. getSimilarityHangul = get Similarity Hangul = 유사성을 얻다 한글

2번, 즉 Hangul의 위치가 맨 뒤로 고정된다면 이름 변경 혹은 새로운 이름을 짓게 될 경우 매우 편할 거라는 생각이 들어요. 하지만 오픈소스 특성상 한국 국적외의 분들도 사용이 가능하니 이 점을 고려한다면 1번도 괜찮은 선택지라고 생각해요. 그래도 역시 한글이라는 특수성을 가진 라이브러리는 한국어를 어느정도 아시는 분들이 사용하실거라는 생각하여 2번을 추천해요.

이외에도 2번을 추천하는 이유는 코드 레벨에서의 작업 때문도 있는데요. 예를 들어, getHangulSimilarity와 getHangulAnything이 있다고 가정해볼게요. 추가로 대다수의 개발자분들이 import 자동완성 기능을 제공하는 IDE를 사용하고 있을거라고 가정할게요.

getHangul을 에디터에서 타이핑한 순간 IDE는 getHangulSimilarity, getHangulAnything 2개를 추천해주는 context를 보게 돼요. 아래의 예시들은 제 로컬 환경에서 관련 함수들을 만들어서 실험해봤어요.
image

context를 확인하면서 내가 정말 원하는 기능을 다시 찾아봐야 한다는 점이 불편해요. 지금은 단 2개 뿐이라서 길게 고민하지 않아도 돼요. 하지만 앞으로 더더욱 다양한 기능들이 생겨 Hangul이라는 명칭이 함수에 붙을 예정인 es-hangul을 생각한다면 그렇게 좋은 방향은 아닌 것 같아요.

물론 Hangul을 건너뛰고 getSi-로 타이핑하면 context가 나오기는 해요.
image
다만, 문제는 좀 더 길고 명확하게 타이핑하지 않으면 추천 context의 우선순위에서 밀린다는 점이에요.

반면에 Hangul을 맨 뒤로 보내볼게요. 보통 라이브러리에서 제공하는 네이밍을 그대로 따라치잖아요? getSi까지 타이핑을 해볼게요.
image

어떤가요? 앞에서 context를 바라보며 1~2초정도 고민을 했었다면 이번에는 단 1초의 고민도 하지 않고 엔터를 눌렀을 거라고 장담해요.

저는 실사용자의 입장에서 생각해봤는데요. 이러한 이유들로 @manudeli 님의 의견에 동의합니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

이전에 동사형이면 좋겠다는 것은 알지만 hangul이 맥락을 넣는다면 아래와 같이 간결하게 표현하는 것은 어떨까요?

@manudeli get을 제거하고 similarityHangul이라고 명명하는 것은 "함수를 동사형으로 만든다"라는 일관성을 더는 지킬수가 없다고 생각해요. (참고)

'Hangul' 이라는 고유명사가 들어가는 경우에만 한정하여 일관성 규칙을 포기하려고 하시는 건지 궁금해요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

zod처럼 Fluent Interface 패턴과 Hangul이라는 단어가 생략된채로 제공하는 두 가지 방식 모두 같이 제공하면 좋다고 생각합니다. 개발자의 입맛에 맞게 사용할 수 있겠네요. 또는, lodash에서 사용하고 있는 방법인 "per method packages"방식으로 제공하는 것도 괜찮을 것 같습니다.

Copy link
Member Author

Choose a reason for hiding this comment

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

바로 위에서 @manudeli 님이 제안주신 컨벤션에 매우 동의합니다!
실제 예시를 보니, 더욱 직관적으로 이해할 수 있었습니다. 감사합니다!
7월1일까지, 다른 분들 추가 의견 없으면 @manudeli 님이 제안주신의견으로 진행하면 좋을 것 같아요

Copy link
Contributor

@Collection50 Collection50 Jul 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

저도 완전히 동의하는 바입니다 !
그렇다면 #138 #130 등을 포함한 모든 메서드 네이밍에서 hangul을 제외하는 것으로 이해하면 될까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

네네! 외부로 노출되는 메서드는 hangul을 제외하는 방향으로 진행하시죠! 다들 너무 감사합니다

Copy link
Contributor

Choose a reason for hiding this comment

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

#138 에서 hangul을 제외하는 작업도 같이 진행해도 괜찮을까요??
PR의 이름, 목적과 잘 부합하는 것 같아요 👍



// Don't
function disassemble()
// Do
function disassembleHangul()
```

okinawaa marked this conversation as resolved.
Show resolved Hide resolved
- [래퍼런스](https://github.com/toss/es-hangul/issues/121)