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

Extract report store API #549

Merged
merged 21 commits into from
Sep 3, 2024
Merged

Extract report store API #549

merged 21 commits into from
Sep 3, 2024

Conversation

bamx23
Copy link
Collaborator

@bamx23 bamx23 commented Aug 28, 2024

There is a set of methods in KSCrash API that expect install or report store setup.

But we don't actually need any global state to work with crash reports. So this PR adds a separate reports store class that extracts all public API for reports reading.

@bamx23 bamx23 marked this pull request as ready for review August 31, 2024 17:36
Copy link
Collaborator

@GLinnik21 GLinnik21 left a comment

Choose a reason for hiding this comment

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

Great stateless implementation!

Sources/KSCrashRecording/include/KSCrashConfiguration.h Outdated Show resolved Hide resolved
Sources/KSCrashRecording/KSCrashConfiguration.m Outdated Show resolved Hide resolved
Sources/KSCrashRecording/include/KSCrashReportStore.h Outdated Show resolved Hide resolved
*
* @param onCompletion Called when sending is complete (nil = ignore).
*/
- (void)sendAllReportsWithCompletion:(nullable KSCrashReportFilterCompletion)onCompletion;
Copy link
Collaborator

Choose a reason for hiding this comment

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

In swift it looks like open func sendAllReports() async throws -> ([any KSCrashReport], Bool). Perhaps it would be useful to document the "return value" as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't think it's a right way... Tried playing with the docs but wasn't happy with the results. We can iterate on this later, it's good that API itself looks fine. Maybe we can drop "isSuccess" and assume that "no error == success". This would make API more clean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we can drop "isSuccess" and assume that "no error == success". This would make API more clean.

Yeah, I like this approach

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will do separately, it would bring way too many changes to this PR

Sources/KSCrashRecording/include/KSCrashReportStoreC.h Outdated Show resolved Hide resolved
@bamx23 bamx23 merged commit e680dd9 into master Sep 3, 2024
29 checks passed
@bamx23 bamx23 deleted the report-store branch September 3, 2024 20:14
@bamx23 bamx23 mentioned this pull request Sep 3, 2024
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