-
Notifications
You must be signed in to change notification settings - Fork 0
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
SwiftDataStore #4
base: work/swift-data-repo-tim
Are you sure you want to change the base?
Conversation
# Conflicts: # Sources/SwiftRepo/Repository/StoreModel.swift
# Conflicts: # Sources/SwiftRepo/Repository/Repository/PagedQueryRepository.swift
@@ -17,7 +17,7 @@ public struct DefaultError<UIErrorType: UIError>: AppError { | |||
intent: ErrorIntent = .indispensable | |||
) { | |||
self.uiError = uiError | |||
self.error = error | |||
self.error = error as? NSError |
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.
If we're going to do this, the initializer should specify NSError
rather than Error
so that other error types don't slip through the cracks. But ideally this would work with any Error
. Remind me why we need Equatable
?
public func set(key: Key, value: Value?) throws -> Value? { | ||
if let value { | ||
try modelContext.insert(TimestampedValue(key: key, value: value, encoder: encoder)) | ||
try modelContext.save() |
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.
Why save here? By default, I think apps should rely on auto-save. If there's a need to save on every insertion, we can add a configuration option to the store.
// MARK: - Constants | ||
|
||
@Model | ||
class TimestampedValue { |
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.
I'd make this a top-level internal type and conform to StoreModel
—just for consistency and easier potential reuse.
try evict(for: key) | ||
modelContext.insert(value) | ||
} | ||
try modelContext.save() |
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.
Same concern about saving
|
||
@MainActor | ||
public func clear() async throws { | ||
try modelContext.delete(model: Value.self) |
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.
Shouldn't we clear the timestamp store here as well?
private let timestampStore: PersistentStore<Model.Key, UUID> | ||
|
||
@MainActor | ||
private var modelContext: ModelContext { |
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.
Does lazy var
not work for this scenario?
No description provided.