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

[PC-000] suspendRunCatching 추가 #26

Merged
merged 2 commits into from
Jan 8, 2025
Merged

Conversation

tgyuuAn
Copy link
Member

@tgyuuAn tgyuuAn commented Jan 7, 2025

1. ⭐️ 변경된 내용

  • Suspend함수에서 runCatching을 할 때 Cancellation이 전파되지 않음을 파악했어요!

2. 🖼️ 스크린샷(선택)

image

3. 💡 알게된 부분

Github를 돌아다니다가 현우님이 아래와 같은 PR을 작성해주신 것을 확인했어요.

ku-ring/KU-Ring-Android#405

너무 흥미로워서 공유합니다!

  • runCatching은 Cancellation을 전파하지 못한다...!

레퍼런스

@tgyuuAn tgyuuAn added 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥 리팩토링 🧰 동작의 변화는 없지만 가독성, 유지보수 측면에서의 코드 개선 🧰 새롭게 알게 됨 📖 프로젝트를 진행하며 새롭게 알게된 부분 📖 ㅌㄱ태규 ☀️ 훗날 크게될 ENFP 남성, tgyuuAn labels Jan 7, 2025
@tgyuuAn tgyuuAn requested a review from sksowk156 January 7, 2025 05:02
@tgyuuAn tgyuuAn self-assigned this Jan 7, 2025
Copy link
Contributor

@sksowk156 sksowk156 left a comment

Choose a reason for hiding this comment

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

태규님 덕분에 좋은 정보를 많이 배워가네요 👍 멋지십니다 정말

@@ -31,8 +32,8 @@ class TermsRepositoryImpl @Inject constructor(
localTermDataSource.clearAndInsertTerms(termsEntity)
}

override suspend fun getTerms(): Result<List<Term>> = runCatching {
override suspend fun getTerms(): Result<List<Term>> = suspendRunCatching {
Copy link
Contributor

Choose a reason for hiding this comment

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

저번 PR에서 제가 달았던 코멘트의 의미는 get보다 의미 범위가 조금 더 좁은 단어를 말하는 거였어요!ㅎㅎ 제가 요즘 여기저기서 스코프라는 단어를 쓸 일이 좀 있었는데, 아무 생각없이 스코프라는 단어를 썼던 것 같아요 허허... 다음 PR부턴 조금 더 명확한 표현을 쓰도록 할게요...!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

아하 괜찮습니다!!

좀 더 좁은 의미가 흠.. 네이밍이 생각이 ㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠㅠ

localTermDataSource.getTerms()
.map { it.toDomain() }
.map(TermEntity::toDomain)
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 👍

@sksowk156 sksowk156 added 머지 해도될듯염🌟 현재 코드를 기존 코드에 합쳐도 될 것 같다라고 판단..! 🌟 and removed 리뷰 원해요🔥 피어의 리뷰를 기다리는 ing.. 🔥 labels Jan 7, 2025
@tgyuuAn tgyuuAn merged commit dba8062 into develop Jan 8, 2025
1 check passed
@tgyuuAn tgyuuAn deleted the refactor/tgyuu/PC-000 branch January 8, 2025 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ㅌㄱ태규 ☀️ 훗날 크게될 ENFP 남성, tgyuuAn 리팩토링 🧰 동작의 변화는 없지만 가독성, 유지보수 측면에서의 코드 개선 🧰 머지 해도될듯염🌟 현재 코드를 기존 코드에 합쳐도 될 것 같다라고 판단..! 🌟 새롭게 알게 됨 📖 프로젝트를 진행하며 새롭게 알게된 부분 📖
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants