-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feature/#85 컨퍼런스 상세 정보 조회 기능추가 #94
The head ref may contain hidden characters: "Feature/#85-\uCEE8\uD37C\uB7F0\uC2A4_\uC0C1\uC138_\uC815\uBCF4_\uC870\uD68C_\uAE30\uB2A5\uCD94\uAC00"
Conversation
- 행사가 존재하지 않는 경우 예외처리 #85
현장 리뷰 완료요~ |
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.
수고하셨습니다~!!
@@ -1,16 +1,31 @@ | |||
package com.emmsale.event.application.dto; | |||
|
|||
import com.emmsale.event.domain.Event; | |||
import java.time.format.DateTimeFormatter; | |||
import lombok.Getter; | |||
import lombok.RequiredArgsConstructor; | |||
|
|||
@RequiredArgsConstructor | |||
@Getter | |||
public class EventResponse { |
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.
행사 목록 조회 API에서 만든 EventResponse랑 같은 이름을 사용하고 있어서 조율이 필요할 것 같습니다!
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.
detailResponse로 변경하였습니다.
|
||
@Override | ||
public BaseExceptionType exceptionType() { | ||
return null; |
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.
null 대신 eventExceptionType을 반환하도록 해야 할 것 같습니다!
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.
리뷰할 때 놓친 게 있었네요...!😥
index.adoc이 업데이트가 안된 것 같습니다!
작성해주세요~~!
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.
크게 수정할 점은 보이지 않네요!
수고하셨습니다~
@Getter | ||
public class EventDetailResponse { | ||
|
||
private static final DateTimeFormatter DATE_TIME_FORMATTER = DateTimeFormatter.ofPattern( |
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.
정적팩터리 메서드에서 LocaldTateTIme을 String으로 바꾸는데 사용했습니댜.
근데, 지금은 @JsonProperty를 이용하여 포맷팅하도록 리팩터링 했습니다.
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.
지웠습니당
@Autowired | ||
private EventRepository eventRepository; | ||
|
||
@Nested |
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.
저는 Nested를 성격이 다른 테스트가 한 테스트 파일에 있을 때 같은 성격의 테스트끼리 그룹화하기 위해 사용하는데 여기서는 굳이 필요해 보이지 않습니다.
혹시 Nested를 사용하신 이유나 기준이 있으신가요?
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.
저는 Nested를 묶는 기준이 하나의 메서드에 대한 테스트일 때 묶습니다.
이 경우에는 findEvent에 대한 테스트끼리 묶었다고 볼 수 있죠.
지금 저 테스트들만 있는데 묶은 이유는 추후 EventService에서 많은 테스트들이 생기기 때문에 미리 묶어둔 겁니당.
추가로 .http 테스트 만들기로 했었어서 그것까지 부탁드려요~ |
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.
수고하셨습니다~!
#️⃣연관된 이슈
📝작업 내용
close #85