-
Notifications
You must be signed in to change notification settings - Fork 0
네트워크 Exception 처리 방식 변경 #442
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
Conversation
plgafhd
commented
Jun 19, 2025
- 전역적인 network exception 처리 개선 #432 의 변경 내용을 되돌리고, 0510 스프린트 대화 내용을 토대로 다시 변경
# Conflicts: # app/src/main/java/com/wafflestudio/snutt2/views/RootActivityViewModel.kt
app/src/main/java/com/wafflestudio/snutt2/lib/network/Result.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/lib/network/Result.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/test/TestRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/test/TestViewModel.kt
Outdated
Show resolved
Hide resolved
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.
지난번에 대화한 것 해석 + GPT/Gemini의 조언을 받은 결과물...
사실 걸리는 점들이 좀 많긴 한데 일단 PR 올려보고 같이 얘기하고 싶어서 올렸습니다
app/src/main/java/com/wafflestudio/snutt2/test/TestViewModel.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/test/TestRepository.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/lib/network/Result.kt
Outdated
Show resolved
Hide resolved
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.
일단 리뷰 반영 + 하다가 고민 2개 생겨서 남겨봤어
app/src/main/java/com/wafflestudio/snutt2/lib/network/ExceptionMapper.kt
Show resolved
Hide resolved
app/src/main/java/com/wafflestudio/snutt2/lib/network/ExceptionMapper.kt
Show resolved
Hide resolved
엥 이거 build 왜 자꾸 터지지 |
fun getDisplayMessage(error: DomainError): String? { | ||
return when (error) { | ||
is NetworkDisconnect -> context.getString(R.string.error_no_network) | ||
is ServerFault -> context.getString(R.string.error_server_fault) | ||
is NoAdminPrivilege -> context.getString(R.string.error_no_admin_privilege) | ||
is UnknownApp -> context.getString(R.string.error_unknown_app) | ||
is AuthError.WrongApiKey -> context.getString(R.string.error_wrong_api_key) | ||
is AuthError.NoUserToken -> context.getString(R.string.error_no_user_token) | ||
is AuthError.WrongUserToken -> context.getString(R.string.error_wrong_user_token) | ||
is Unknown -> context.getString(R.string.error_unknown) | ||
else -> 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.
이거 else 이면 error.displayMessage 반환해야, ui 의 displayMessage를 결정하는 모든 로직이 DisplayMessageResolver 안에 전부 들어갈 것 같음
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.
아 그러게 그랬으면 되네? ㅋㅋㅋㅋ
결국 나중에 displayMessageResolver.getDisplayMessage(error) ?: error.displayMessage
하니까
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.
이거 지금 NetworkDisconnect 나 ServerFault 같은 애들은 displayMessage가 "" 로 되어 있는 거지?
아 이거 뭔가
sealed interface DomainError {
val displayMessage: String?
}
이거 이렇게 nullable로 바꾸고, displayMessage 없는 애들은 null 로 주고 싶은데
이렇게 할려면 sealed interface DomainError 안에 다 넣어야되네
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.
그치 이 getDisplayMessage
가 displayMessage가 없거나, 서버 displayMessage랑 클라의 displayMessage가 다른 경우를 커버해주는 의도로 했어
그리고 null이 가능하다는것 자체가 좀 별로기도 하구... 서버 displayMessage랑 클라의 displayMessage가 다른 경우
도 있어서 그냥 null/empty로 판단하는게 아니라 DomainError Type에 따라 주는게 낫다고 보긴 했어
그리고 사실 그게 형의 의도라고 생각하긴 했는데... ㅋㅋㅋㅋ
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.
nullable 하는 순간 많은 순간에서 귀찮을 거라 지금 한게 제일 실용적일 것 같긴 하다
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.
일단고~ 수고했음
:ablobcheer: 요거 Test Repository/ViewModel/Screen 만든 의도가 다른 화면 리팩토링 할 때 참고가 되면 좋을 것 같아서 |