-
Notifications
You must be signed in to change notification settings - Fork 83
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
[engine] getCommitRaws 함수 개선 #753
Conversation
[engine] getCommitRaws 함수 개선
[engine] getCommitRaws 테스트코드 보완
[engine] getCommitRaws 테스트케이스 추가
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.
LGGGTM!!
PR description 정리도 엄청 잘 되어 있네요!!!! PR writing의 정석 같은 느낌입니다 💯💯💯
(사소하지만, 직접적인 성능 비교할 때는 얼만큼 개선이 되었는지 비율을 표시해주는 것도 좋은 방법입니다.)
separator관련해서는,
가급적이면 코드의 가독성, 뒷 사람이 잘 이해할 수 있도록 하는 유지보수성 등을 고려해서 짠다면, \n 같은게 좋긴 하겠지만,
그렇게 했을 떄 만약 performance저하가 심하게 일어난다면, 현재 방법이 좋을 수도 있겠지요.
고생하셨습니다👍👍👍 |
@yoouyeon 님,
기존 코드들을 보면 pretty=fuller 옵션을 주고,
End of message인 \n 이 나올 때 까지 message 로 인식합니다. (fuller가 알아서 body는 indentation 해줌) githru-vscode-ext/packages/analysis-engine/src/parser.ts Lines 68 to 80 in 68d4aaa
저희도 옵션을 잘 활용하면 body 에다가 앞에 space를 주면서 다른 내용들(numstat)과 구분이 가게 할 수 있습니다.
위처럼 %w를 사용하면 가능하구요 (한참 찾았네요;;;) |
Related issue
Result
getCommitRaws의 복잡한 파싱 로직을 단순화하고, 실행 속도를 (조금) 단축시켰습니다.
AS-IS
git log format
getCommitRaws
실행 시간TO-BE
git log format
getCommitRaws
실행 시간테스트 결과
Work list
getGitLog
에서 받아오는 git log의 format 변경getCommitRaws
의 파싱 로직 수정([engine] getCommitRaws 함수 개선 #680)
([engine] getCommitRaws 테스트코드 보완 #708, [engine] getCommitRaws 테스트케이스 추가 #735)
Discussion
지금은 Separator를 난수 문자열로 사용하고 있기 때문에
의 문제가 있어 Separator를 다른 것으로 바꿀 필요가 있습니다.
대안으로 new line (
\n
)을 사용하는 방법이 있고, 지금까지는 가장 좋은 대안이라는 생각이 드는데요. 🥲테스트해본 결과 (#708 (comment))
\n
을 사용하려면 git log에서 diffstat을 따로 불러오는 등의 추가적인 작업이 많이 필요할 듯 하여,,Separator 변경은 후속 이슈로 남겨두고 다른 대안이나 diffstat을 따로 불러오는 것에 대한 충분한 조사와 논의 후에 적용하는 것이 어떨까 싶습니다.. 🥹🥹