[Bug]: removeLastHangulCharacter 함수에서 disassembledGroups의 길이가 4개인 경우 에러가 발생합니다.

jgjgill commented 3 months ago

Bug description

removeLastHangulCharacter: 인자로 주어진 한글 문자열에서 가장 마지막 문자 하나를 제거하여 반환

현재 removeLastHangulCharacter에서 withoutLastCharacter를 구할 때 map 함수 내부에서 first, second, last로 구성되어 있어요. 하지만 last에서 생각대로 동작하지 못하는 부분을 발견했습니다.

disassembleHangulToGroups 함수의 테스트 예시로 값을 넣었을 경우 ['ㄱ', 'ㅏ', 'ㅂ', 'ㅅ']을 기대하고 있습니다.

하지만 실제 combineHangulCharacter로 다시 결합시킬 때는 값을 받고 싶으면 'ㄱ', 'ㅏ', 'ㅂㅅ'을 입력받기를 기대하고 있습니다.

그래서 다음과 같이 테스트를 진행했을 때 에러가 발생해요.

테스트값: 안녕하세요 값이
예상 값: 안녕하세요 값ㅇ
실제 값: 안녕하세요 갑ㅇ

콘솔로 확인해보면 last에서 ㅅ이 생략되고 있습니다.

정리하면 종성으로 겹받침이 오면 withoutLastCharacter 변수를 구할 때 map 함수에서 생략되는 부분이 발생해요.

Expected behavior

No response

To Reproduce

종성에 겹받침이 오는 문자가 마지막 문자가 아닌 경우 에러가 발생합니다.

ex) 안녕하세요 많이, 안녕하세요 값이

Possible Solution

나머지 매개변수를 활용해서 마지막은 join으로 합쳐서 last를 구성하는 것을 생각했어요.

const withoutLastCharacter = disassembledGroups
  .filter(v => v !== lastCharacter)
  .map(([first, middle, ...rest]) => {
    const last = rest.join('');

    if (middle != null) {
      return combineHangulCharacter(first, middle, last);
    }

    return first;
  });

안녕하세요 많이로 테스트 진행할 경우

예상 값: 안녕하세요 많ㅇ
실제 값: 안녕하세요 많ㅇ

etc.

라이브러리를 보면서 많이 배우고 있습니다..! 감사합니다! 🙇‍♂️

crucifyer commented 3 months ago

요 pr 로 해결 되겠네요 :) https://github.com/toss/es-hangul/pull/124/commits/91377c54236f2f4568eb7051843a9a53864dbca0

jgjgill commented 3 months ago

@crucifyer 안녕하세요! 해당 부분 이슈나 PR이 없어서 작업이 진행된 줄 몰랐네요..! 😅 참고해주신 커밋 확인했습니다!

해당 파일에서 문제가 되는 withoutLastCharacter을 없애면 될 것 같네요..! 여기서 withoutLastCharacter이라는 변수명은 유지하면 어떨까요? 변수명을 통해 더 이해하기 쉬울 것 같아요. 그리고 나머지 부분은 따로 변경할 필요없이 기존 코드와 유사하게 구성해도 괜찮을 것 같은데 어떻게 생각하시나요?

제안하는 코드

export function removeLastHangulCharacter(words: string) {
  const disassembledGroups = disassembleHangulToGroups(words);
  const lastCharacter = disassembledGroups[disassembledGroups.length - 1];

  if (lastCharacter == null) {
    return '';
  }

  const withoutLastCharacter = words.substring(0, words.length - 1);
  const [[first, middle, last]] = excludeLastElement(lastCharacter);
  const result = middle != null ? combineHangulCharacter(first, middle, last) : first;

  return [withoutLastCharacter, result].join('');
}

우선 해당 이슈는 올려주신 PR이 머지되거나 따로 메인테이너분께서 안내해주시기 전까지는 열어두겠습니다..! 감사합니다..! 🙇‍♂️

jgjgill commented 2 months ago

해당 PR에서 개선되어서 close 합니다..!

toss / es-hangul