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

Change hard-coded preview data to not hard-coded #25

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from

Conversation

minacle
Copy link
Collaborator

@minacle minacle commented May 26, 2022

No description provided.

@minacle minacle force-pushed the feature/preview-manifest branch 3 times, most recently from 43d8d21 to f242c4d Compare May 27, 2022 02:03
@minacle
Copy link
Collaborator Author

minacle commented May 27, 2022

이렇게 되면 함수를 파트1 파트2 처럼 나눌 수밖에 없어지니 린트는 포기하겠습니다.

@minacle minacle requested a review from sinoru May 27, 2022 02:21
@sinoru
Copy link
Collaborator

sinoru commented May 27, 2022

이렇게 되면 함수를 파트1 파트2 처럼 나눌 수밖에 없어지니 린트는 포기하겠습니다.

@minacle 제안드리자면, 어차피 프리뷰 데이터인 만큼 Reflection을 써서 값을 대입하는 방법도 있습니다.

@minacle minacle force-pushed the feature/preview-manifest branch from 1a1983b to c1376f4 Compare May 27, 2022 12:31
@minacle minacle force-pushed the feature/preview-manifest branch 3 times, most recently from 6c40ef4 to c96adf3 Compare May 27, 2022 14:28
@minacle minacle force-pushed the feature/preview-manifest branch from c96adf3 to 4986129 Compare May 27, 2022 20:34
.swiftlint.yml Outdated
@@ -10,6 +10,10 @@ opt_in_rules:
- empty_collection_literal
- empty_string

cyclomatic_complexity:
warning: 20
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
warning: 20
warning: 15

Comment on lines +35 to +36
context: NSManagedObjectContext? = nil,
save: Bool = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

save parameter를 받는 이유가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

외부에서 context를 제공했을 경우 여러 파일을 한꺼번에 읽어들인 뒤 직접 한 번만 저장할 수도 있습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

context를 받지 않거나, default 값에서 nil 대신 아에 background context를 생성하는 것도 좋을 것 같습니다.

@@ -0,0 +1,262 @@
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
Copy link
Collaborator

Choose a reason for hiding this comment

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

plist를 선택한 이유가 있을까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

간편하기 때문입니다.

Comment on lines +52 to +82
let state = _PreviewDataInjectionState(bundle: bundle)
for (userID, previewUserDetails) in previewManifest.userDetails {
state.userDetail = nil
for previewUserDetail in previewUserDetails {
let userDetail = _injectPreviewUserDetail(previewUserDetail, state: state, context: context)
if state.userIDToUserDetail[userID] == nil {
state.userIDToUserDetail[userID] = userDetail
}
state.userDetail = userDetail
}
}
for (userID, userDetail) in state.userIDToUserDetail {
state.userDetail = userDetail
_injectPreviewUser(id: userID, state: state, context: context)
}
for userID in previewManifest.accounts {
guard let userDetail = state.userIDToUserDetail[userID] else {
continue
}
state.userDetail = userDetail
_injectPreviewAccount(userID: userID, state: state, context: context)
}
for (userID, previewUserDataAssets) in previewManifest.dataAssets {
guard let userDetail = state.userIDToUserDetail[userID] else {
continue
}
state.userDetail = userDetail
for previewUserDataAsset in previewUserDataAssets {
_injectPreviewUserDataAsset(previewUserDataAsset, state: state, context: context)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

state를 사용하여 넣는 형태가 필요에 비해 과한 것 같습니다.
좀 더 단순하게 할 수 있을 것 같습니다.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

좀 더 단순하게 하면 복잡도가 30에 달합니다.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants