-
Notifications
You must be signed in to change notification settings - Fork 1
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: Comment API에서 도메인명 안바뀐 부분 수정 #609 #610
fix: Comment API에서 도메인명 안바뀐 부분 수정 #609 #610
Conversation
Test Results 34 files 34 suites 6s ⏱️ Results for commit 616aa55. ♻️ This comment has been updated with latest results. |
🌻Test Coverage Report
|
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.
수정하시느라 수고하셨어용~
한 가지 코멘트 남겼는데, 제안이라 approve합니다!
확인해보시고 선택적으로 반영해주세요:>
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.
테스트는 하나의 파일에서 만들려다가 테스트 데이터 구성에 복잡함을 느껴서, 구현의 편리성을 위해 다른 파일로 분리해서 작업했습니다.
해당 테스트 파일에 기존 컨트롤로 테스트가 전부 있는게 아닌 변경에 대한 테스트만 일부 있군요! 처음에 얼핏 봤을 때는 모든 엔드포인트 테스트가 있을거라고 생각이 들었던 것 같아요. 또한, v2를 제거할땐 해당 테스트파일과 기존 컨트롤러 테스트 파일 총 2 군데 변경 지점이 생겨서 다른 개발자가 추후에 수정하게 된다면 헷갈릴 여지가 있다고 생각이 들었어요!
그래서 한 가지 제안 드리자면, 기존에 있는 관련 테스트에 필요한 요청 데이터와 검증부만 추가하는건 어떻게 생각하시나요?
제가 전에 썼던 방식이기도 한데요, 변경 지점이 하나로 분명하니까 편리하지 않을까 싶어서 제안드립니당
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.
해당 테스트 파일에 기존 컨트롤로 테스트가 전부 있는게 아닌 변경에 대한 테스트만 일부 있군요! 처음에 얼핏 봤을 때는 모든 엔드포인트 테스트가 있을거라고 생각이 들었던 것 같아요. 또한, v2를 제거할땐 해당 테스트파일과 기존 컨트롤러 테스트 파일 총 2 군데 변경 지점이 생겨서 다른 개발자가 추후에 수정하게 된다면 헷갈릴 여지가 있다고 생각이 들었어요!
결국에 나중에 수정할 때, (새로운 파일을 살리는 경우) 새로운 파일에 원래 테스트를 넣어야 하므로, 지금 추가해놓는 게 낫겠네요! 수정했습니다.
그래서 한 가지 제안 드리자면, 기존에 있는 관련 테스트에 필요한 요청 데이터와 검증부만 추가하는건 어떻게 생각하시나요?
제가 전에 썼던 방식이기도 한데요, 변경 지점이 하나로 분명하니까 편리하지 않을까 싶어서 제안드립니당
이 방법을 시도해보려 했으나, createComment
에서는 괜찮은데, createCommentFail
에는 MethodSource
를 사용하기 때문에 하나의 함수에서 처리하지 못하고, readCommentsByMomentId
와 readCommentsByMomentIdFail
는 메서드 이름의 MomentId
가 StaccatoId
로 바뀌어야 하기 때문에, 해당 방법을 사용하지 않았습니다.
그리고 이 새로 생겨난 메서드들까지 원래 파일에서 관리하면 오히려 나중에 제거할때 훨씬 복잡해질 것 같아, 새로운 파일로 분리해서 만들었습니다. 혹시 제가 생각하지 못한 더 좋은 방법이 있다면, 공유 부탁드립니다!
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.
이 방법을 시도해보려 했으나, createComment에서는 괜찮은데, createCommentFail에는 MethodSource를 사용하기 때문에 하나의 함수에서 처리하지 못하고, readCommentsByMomentId와 readCommentsByMomentIdFail는 메서드 이름의 MomentId가 StaccatoId로 바뀌어야 하기 때문에, 해당 방법을 사용하지 않았습니다.
그렇겠네요! 두 메서드는 어쩔 수 없을 것 같아요! 반영하시느라 수고하셨슴다~!~!
⭐️ Issue Number
🚩 Summary
POST /comments
에서 사용하는CommentRequest
내부에momentId
가 있음->
staccatoId
를 사용하는 새로운CommentRequestV2
Dto 생성->
CommentRequestV2
를 사용하는POST /comments/v2
생성GET /comments
의RequestParam
에momentId
가 있음->
staccatoId
를 사용하는GET /comments/v2
생성🙂 To Reviewer
📋 To Do