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

Create structured BugsnagStackframe class #528

Merged
merged 2 commits into from
Apr 9, 2020

Conversation

fractalwrench
Copy link
Contributor

@fractalwrench fractalwrench commented Apr 6, 2020

Goal

Creates a publicly accessible BugsnagStackframe class which holds fields in properties, rather than an NSDictionary.

A project-visible BugsnagStacktrace has been created to sanitise the stacktrace - notably by trimming its length to <200 frames, as per the spec.

The BugsnagStackframe class will eventually be present on the BugsnagError and BugsnagThread classes, which will be implemented in future PRs. For now the JSON serializer has been updated to use the object internally.

Changeset

  • Add properties to BugsnagStackframe which can be edited by the user. Note that these deviate from the spec
  • Create BugsnagStacktrace which limits the number of BugsnagStackframe to <200, and encapsulates stacktraces to allow any similar modifications in future
  • Updated BugsnagEvent JSON serialization to use BugsnagStacktrace internally
  • Refactored BSGFormatFrame to serializers in BugsnagStackframe/BugsnagStacktrace

Tests

Added unit tests to verify that stacktraces can be parsed from a dictionary/serialized to JSON. Relied on existing unit test coverage to verify the event JSON remains unchanged.

@fractalwrench fractalwrench force-pushed the v6-structured-stacktrace branch from 90430b8 to 2cc5cf3 Compare April 6, 2020 16:40
@fractalwrench fractalwrench changed the title Convert event.device from NSDictionary to a structured class Convert Stacktrace from NSDictionary to a structured class Apr 7, 2020
@fractalwrench fractalwrench force-pushed the v6-structured-stacktrace branch from 2cc5cf3 to a26ae8c Compare April 7, 2020 09:16
@fractalwrench fractalwrench changed the title Convert Stacktrace from NSDictionary to a structured class Create structured BugsnagStacktrace class Apr 7, 2020
@fractalwrench fractalwrench changed the title Create structured BugsnagStacktrace class Create structured BugsnagStackframe class Apr 7, 2020
@fractalwrench fractalwrench force-pushed the v6-structured-stacktrace branch from a26ae8c to 413e92b Compare April 7, 2020 09:45
@fractalwrench fractalwrench marked this pull request as ready for review April 7, 2020 09:52
Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

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

Looks good. Couple of comments very similar to the event.device ones, unsurprisingly given the sequence of commits. Also, small Xcode nitpick: it looks like the Event class has moved out of sequence to the top of the Payload group.

Source/BugsnagStacktrace.m Outdated Show resolved Hide resolved
Source/BugsnagStacktrace.m Show resolved Hide resolved
@fractalwrench fractalwrench force-pushed the v6-structured-stacktrace branch from 413e92b to c50dbaa Compare April 8, 2020 15:12
@fractalwrench
Copy link
Contributor Author

Addressed inline comments and resolved the XCode source code organisation nit. I've also rebased against v6 to fix the merge conflicts.

Copy link
Contributor

@robinmacharg robinmacharg left a comment

Choose a reason for hiding this comment

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

LGTM.

@fractalwrench fractalwrench force-pushed the v6-structured-stacktrace branch from c50dbaa to 8402c86 Compare April 9, 2020 10:32
@fractalwrench fractalwrench merged commit b8229be into v6 Apr 9, 2020
@fractalwrench fractalwrench deleted the v6-structured-stacktrace branch May 13, 2020 13:49
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