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

Use protocol for crash report type #527

Merged
merged 8 commits into from
Jul 8, 2024
Merged

Use protocol for crash report type #527

merged 8 commits into from
Jul 8, 2024

Conversation

bamx23
Copy link
Collaborator

@bamx23 bamx23 commented Jul 7, 2024

The 3-values crash report type wasn't a good idea form an API side of view as some users were confused that stringValue would automatically convert crash report to string.

This PR replaces KSCrashReport class with an abstract protocol. This also allows writing custom filters with user-defined representations of crash report (e.g. protobuf or something else).

@bamx23 bamx23 requested a review from GLinnik21 July 7, 2024 13:26
@GLinnik21
Copy link
Collaborator

Addresses #525 (comment)

@GLinnik21
Copy link
Collaborator

Ok, now we have to use isKindOfClass: everywhere, but I believe it's a fair tradeoff.

The bigger question is how the value looks. In Swift, it will be Any. This means that the user now has to know exactly which class to cast the value to. Previously, the user had three options. Alternatively, the user can cast the CrashReport protocol to a concrete implementation.

I'm not sure which is more confusing, the new implementation with Any or the current one.

@bamx23
Copy link
Collaborator Author

bamx23 commented Jul 7, 2024

I've added some extra text to the protocol docs. In the protocol itself there's a untypedValue that anyone can use and unsafely cast to whatever they want. And for type safety they can cast to a specific implementation first (and a value there is now guaranteed to be non-nil). I've added an example of a switch to the sample.

Also, as only filters can change the type of the report, the KSCrash class now returns dictionary variant directly making it more straight-forward to use that API.

@bamx23 bamx23 merged commit f509b76 into master Jul 8, 2024
26 checks passed
@bamx23 bamx23 deleted the make-report-protocol branch July 8, 2024 20:15
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