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.

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

[Feat/add custom call adapter]: custom call adapter 클래스 생성 #262

Closed
wants to merge 15 commits into from

Conversation

kangyuri1114
Copy link
Member

개요

작업 사항

  • custom call adapter 클래스 생성

변경 사항(optional)

  • CustomCallException
  • CustomResultCall
  • CustomResultCallFactory

스크린샷(optional)

  • 내용을 적어주세요.

@kangyuri1114 kangyuri1114 added 💻feat 새로운 기능 추가 🐰유리 유리 작업 labels Dec 31, 2024
@kangyuri1114 kangyuri1114 self-assigned this Dec 31, 2024
@kangyuri1114 kangyuri1114 requested a review from a team as a code owner December 31, 2024 09:42
Copy link
Member

@kez-lab kez-lab left a comment

Choose a reason for hiding this comment

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

굿 고생하셨습니다, 브랜치명 소문자로 변경 부탁드립니다~!!

추가적으로 이 PR이 바로 병합되면 develop에서 서버통신이 안될겁니다.
따라서 이 브랜치가 Approve 되고 이를 기준으로 응답을 전부 Result로 변경하는 작업 브랜치 파서 작업 후 순차적으로 병합하면 될 것 같네요

Comment on lines 3 to 6
class CustomCallException(
override val message: String,
throwable: Throwable
): Exception(message, throwable)
Copy link
Member

Choose a reason for hiding this comment

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

이름에서 Custom은 확장성에 좋을 것 같지는 않아 빠지는게 좋을 것 같습니다.
이름을 ApiException, NetworkException 등은 어떨까요?

Copy link
Member

Choose a reason for hiding this comment

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

추가적으로 throwable을 직접 들고있는 구조가 아닌 ErrorResponse를 넘겨주는 구조가 어떨까요?

그렇게 된다면 실패 분기했을 때 Exception의 타입캐스팅을 2번 진행해야하는 수고로움을 덜 수 있을 것 같습니다

예시: Fail -> ApiException 인가? -> 내부 throwable은 어떤 에러인가?

Copy link
Member Author

Choose a reason for hiding this comment

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

지금 통신이 성공해서 response를 받고 있지만 body가 빈 경우도 ApiException을 발생중인데 그러면 이경우는 errorResponse가 아닌 BaseResponse 형식일텐데 이때는 어떻게 대응해야 할까요??

Copy link
Member

Choose a reason for hiding this comment

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

body가 빈 경우도 ApiException을 ErrorResponse를 기반으로 발생 하면 되지 않나요?

class NetworkException(
    val errorReponse: ErrorResponse,
    val throwable: Throwable
): Exception(throwable)

#262 (comment)

여기 남긴 것 처럼 대응하면 될 것 같아요
혹은 추가적으로 getEmptyErrorReponse() 를 만드셔도 괜찮고요

Comment on lines 85 to 89
// errorBody가 비어있는 경우
if(response.errorBody() == null) {
callback.onResponse( this@CustomResultCall,
Response.success(Result.failure(CustomCallException("errorBody가 비었습니다.", HttpException(response))))
)
Copy link
Member

Choose a reason for hiding this comment

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

Error Response가 생긴다면 errorBody를 저희가 꺼내서 사용할 필요도 없어질 것 같아서 이런 부분도 더 간단하게 처리가 가능할 것 같네요

if (responseBody == null) {
    val errorBody = getErrorResponse(response) // getErrorResponse 는 ErrorResponse를 반환하는 함수
    callback.onResponse(
        this@ResultCall,
        Response.success(Result.failure(ApiException(response.code(), errorBody))),
    )
} else {
    callback.onResponse(
        this@ResultCall,
        Response.success(
            response.code(),
            Result.success(responseBody),
        ),
    )
}

Copy link
Member Author

Choose a reason for hiding this comment

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

음.. 이 부분이 이해안되는데 다시 설명해주실 수 있나요??
말씀하신 부분은 errorBody를 꺼낼 필요가 없다고 하셨는데 작성해주신 코드 보면 어차피 getErrorResponse라는 함수를 직접 만들어야 하는 건데 그러면 getErrorResponse함수 자체가 errorBody를 꺼내는 함수인 것 아닌가요??

Copy link
Member

Choose a reason for hiding this comment

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

오 넵 이 부분 설명이 좀 미흡했네요, errorBody를 꺼내고 ErrorResponse로 협의된 에러 객체를 내려주면 되고 현재는 대응된 것으로 보이네요

예시

private fun getErrorResponse(response: Response<T>) = ErrorResponse(
        statusCode = response.code(),
        errorCode = NetWorkErrorCode.TEMPORARY_ERROR,
        message = "알 수 없는 에러"
    )

@kez-lab
Copy link
Member

kez-lab commented Jan 4, 2025

브랜치 명은 제가 수정해두겠습니다

@kez-lab kez-lab closed this Jan 4, 2025
@kez-lab kez-lab deleted the feat/add-CustomCallAdapter branch January 4, 2025 05:32
@kez-lab kez-lab restored the feat/add-CustomCallAdapter branch January 4, 2025 05:32
@kez-lab kez-lab reopened this Jan 4, 2025
@kez-lab
Copy link
Member

kez-lab commented Jan 4, 2025

기존게 삭제가 되네요 브랜치명은 PR 끝나고 바꾸겠슴다

@kez-lab
Copy link
Member

kez-lab commented Jan 4, 2025

고생하셨습니다!
브랜치명 수정을 위해서 해당 PR은 클로즈할게요!

@kez-lab kez-lab closed this Jan 4, 2025
@kez-lab kez-lab deleted the feat/add-CustomCallAdapter branch January 4, 2025 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐰유리 유리 작업 💻feat 새로운 기능 추가
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feat]: CustomCallAdapter 추가
2 participants