refactor: ContentListFeature 액션 컨벤션 수정 #140
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stealmh
changed the title
stealmh
reviewed
Oct 6, 2024
Comment on lines
-163
to
+168
| case .bottomSheetButtonTapped(let delegate, let content): | ||
| return .run { send in | ||
| await send(.inner(.dismissBottomSheet)) | ||
| await send(.scope(.bottomSheet(delegate: delegate, content: content))) | ||
| } | ||
| case .deleteAlertConfirmTapped: | ||
| case .bottomSheet(let delegate, let content): | ||
| return .concatenate( | ||
| .send(.inner(.바텀시트_해제)), | ||
| .send(.scope(.bottomSheet(delegate: delegate, content: content))) | ||
| ) | ||
| case .컨텐츠_삭제_눌렀을때: |
There was a problem hiding this comment.
여기도 설명필요 최소한 바꿨으면 PR에 before after정도는...
There was a problem hiding this comment.
여기도 설명필요 최소한 바꿨으면 PR에 before after정도는...
넵..
stealmh
reviewed
Oct 6, 2024
Comment on lines
185
to
190
| .run { send in | ||
| for await _ in self.pasteBoard.changes() { | ||
| let url = try await pasteBoard.probableWebURL() | ||
| await send(.delegate(.linkCopyDetected(url)), animation: .pokitSpring) | ||
| } | ||
| } |
There was a problem hiding this comment.
merge형태로 쓸꺼면 얘도 action으로 빼서 깔끔하게 가져가야함
PokitSearchFeature 310번째줄 참고해서 작성해주세요
stealmh
reviewed
Oct 6, 2024
Comment on lines
236
to
246
| case .컨텐츠_삭제_API(id: let id): | ||
| let count = state.domain.contentCount | ||
| let newCount = count - 1 | ||
|
|
||
| return .merge( | ||
| .send(.inner(.컨텐츠_개수_업데이트(newCount))), | ||
| .run { send in | ||
| let _ = try await contentClient.컨텐츠_삭제("\(id)") | ||
| await send(.inner(.컨텐츠_삭제_API_반영(id: id)), animation: .pokitSpring) | ||
| } | ||
| ) |
There was a problem hiding this comment.
기존 방법보다 가독성이 너무 떨어져요
굳이 await하지 않고 동시에 액션 가져가겠다는 걸 이해했는데도 그렇습니다
There was a problem hiding this comment.
기존 방법보다 가독성이 너무 떨어져요 굳이 await하지 않고 동시에 액션 가져가겠다는 걸 이해했는데도 그렇습니다
넵 좀 더 고민해보겠습니다
stealmh
requested changes
Oct 6, 2024
Closed
14 tasks
넵..!! |
stealmh
approved these changes
Oct 15, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#️⃣연관된 이슈
📝작업 내용
merge,concatenate을 사용한 묶음 액션 핸들링 명시화스크린샷 (선택)
💬리뷰 요구사항(선택)
merge,concatenate를 사용하여 여러 액션 실행을 좀 더 명확하게 가져가고자 하였습니다.run액션을 사용한뷰가_나타났을때코드 입니다.await키워드로 인해 순차적으로 실행되는것 처럼 보이나, 각각의 액션 내부에는run액션으로 서로 다른 비동기 작업을 할당 받고 있기 때문에, 병렬적으로 동작합니다.그렇기 때문에 제가 보기엔 액션 흐름 이해가 혼란스럽다고 느껴서
merge,concatenate를 통해 병렬적인지 순차적인지 명시를 하고 싶었습니다.여러 액션을 묶는 것은
merge,concatenate을 활용하고, 꼭 필요한 비동기 작업에만run액션을 쓰고 싶었습니다.run을 썼을때와 동작하는데 있어서는 큰차이가 없지만,merge를 통해 병렬젹으로 동작한다는걸 확실하게 보여줄 수 있다는 생각입니다..!!merge,concatenate가독성을 높이기 위해, 몇몇 액션을 함수로 묶었습니다.close 이슈번호