-
Notifications
You must be signed in to change notification settings - Fork 1
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
기록소 등록 화면에서 홈 화면으로 전환 간의 엔티티 설계 변경 & 테스트 코드 작성 #110
Conversation
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.
대 효 준 🐶
looks good to me ~
public func create(with memorialHouseName: String) async -> Result<Void, MHDataError> { | ||
userDefaults.set( | ||
memorialHouseName, | ||
forKey: Constant.houseNameUserDefaultKey | ||
) | ||
return .success(()) | ||
} |
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.
리턴 타입이 Result<Void, MHDataError>인데 MHDataError를 리턴해주는 부분은 안보이는 것 같습니다!
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.
인정합니다 수정했어요 !
public enum Output: Equatable { | ||
case registerButtonEnabled(isEnabled: Bool) | ||
case moveToHome | ||
case createFailure(errorMessage: String) | ||
} |
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.
Equatable은 언제 사용되는지 알 수 있을까요? (잘 몰라서 ... 질문)
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.
너무 수고 많으셨습니당 .ᐟ.ᐟ 🥹
몇몇 궁금한 점들 질문으로 남겨뒀습니당 !
아 그리고 추가로 맨 처음 등록뷰에서 book 이미지는 수정 아직 안된게 맞죵?
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.
P3: 개인적인 생각인데 MemorialHouseName은 UserDefault에만 저장될게 분명해서 굳이 한 번 추상화를 해줄 필요가 있나..? 라는 생각입니다. 그냥 Storage를 만들지 않고 바로 repository에서 처리해도 되지 않을까..? 라는 생각?? 입니당
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.
P3: 해당 파일은 UserDefault에서 불러오기 및 삭제 등이 실행되는지에 대한 테스트코드인 것 같은데, repository와 storage를 하나로 합치고 둘 중 하나의 테스트만 존재하면 되지 않을까.. 라는 생각이 드네용 ..
let suiteName = UUID().uuidString | ||
let userDefaults = UserDefaults(suiteName: suiteName)! | ||
let storage = UserDefaultsMemorialHouseNameStorage(userDefaults: userDefaults) |
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.
P3: UserDefault를 UUID를 넣어 생성해주는 로직이 들어가게된 이유가 무엇인지 궁금합니다 .ᐟ.ᐟ
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.
테스트 메소드마다 독립된 UserDefaults를 제공해주고 싶었는데,
String처리보다 UUID로 하는게 나을 것 같아 위처럼 작성해두었습니다 !
@testable import MHFoundation | ||
|
||
struct UserDefaultsMemorialHouseNameStorageTest { | ||
@MainActor |
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.
P3: Test 코드에 MainActor가 붙은 이유가 궁금합니다 !
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.
복붙의 흔적이.. 제거했습니다!
|
||
struct UserDefaultsMemorialHouseNameStorageTest { | ||
@MainActor | ||
@Test mutating func test저장소에_기록소_이름을_저장한다() async throws { |
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.
P3: mutating이 붙은 이유가 무엇인가요??
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.
제거했습니다 !!
BookCover(id: UUID(), order: 1, title: "title2", imageURL: nil, color: .blue, category: nil, favorite: false), | ||
BookCover(id: UUID(), order: 2, title: "title3", imageURL: nil, color: .blue, category: nil, favorite: false) | ||
] | ||
|
||
@MainActor |
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.
P3: 이곳도 MainActor가 붙은 이유가 궁금합니다 !
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.
프레젠트 테스트 코드의 경우 메소드들이 MainActor에서 돌고 있어서 테스트도 붙여줘야 했습니다
안 그러면 아래처럼 에러 발생했습니다.. !! 뷰모델 MainActor 제거할 때 테스트도 다시 손 봐야겠네요 😢
Passing argument of non-sendable type 'RegisterViewModel' into main actor-isolated context may introduce data races; this is an error in the Swift 6 language mode
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.
빛이데아효춘무공준
throw MHDataError.noSuchEntity(key: suiteName) | ||
} | ||
|
||
userDefaults.removePersistentDomain(forName: suiteName) |
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.
팁: class를 통해 init deinit에 테스트 전 / 테스트 후 코드를 자동화 가능합니다!
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.
오오 감사합니다 !!
리팩토링 해볼게용 (리팩토링 기간에)
Task { | ||
do { | ||
try await self?.registerButtonTapped(with: memorialHouseName) | ||
self?.output.send(.moveToHome) | ||
} catch { | ||
self?.output.send(.createFailure(errorMessage: error.localizedDescription)) | ||
} | ||
} |
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.
P3. 별도 함수로 빼는 게 좋아보입니다!
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.
수정했습니다 !
#️⃣ 연관된 이슈
⏰ 작업 시간
📝 작업 내용
📸 스크린샷