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

Track whether an error is handled/unhandled #164

Merged
merged 33 commits into from
Oct 2, 2017

Conversation

fractalwrench
Copy link
Contributor

No description provided.

@@ -60,7 +60,8 @@ NSString *_Nonnull BSGFormatSeverity(BSGSeverity severity);
errorMessage:(NSString *_Nonnull)message
configuration:(BugsnagConfiguration *_Nonnull)config
metaData:(NSDictionary *_Nonnull)metaData
severity:(BSGSeverity)severity;
severity:(BSGSeverity)severity
unhandled:(BOOL)unhandled;
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this interface is public, we may want to add a second constructor to avoid making this a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've reverted the signature change here, as the extra parameter wasn't being used within the constructor, and any client calling this method will be passing a handled error.

} else { // the event was unhandled.
_eventHandledState = @{
@"unhandled": @YES,
@"originalSeverity": BSGFormatSeverity(BSGSeverityWarning),
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be error? I'm not sure when this case would happen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - I've updated this so that unhandled events default to a severity of Error.

Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

This looks good. I left a few questions inline.

NSString *reasonType = [BugsnagHandledState stringFromSeverityReason:self.handledState.calculateSeverityReasonType];
severityReason[@"type"] = reasonType;

if (self.handledState.attrKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like attrValue should be checked too, since a dict value can't be nil

bsg_kscrw_i_writeBinaryImages(writer, BSG_KSCrashField_BinaryImages);
// Don't write the binary images for user reported crashes to improve performance
if (crashContext->crash.threadTracingEnabled == true ||
crashContext->crash.crashType != BSG_KSCrashTypeUserReported) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Disabling writing binary images will need to be behind a flag for unity -- we still need binary images for user reported issues on regular cocoa.

bsg_kscrw_i_writeError(writer, BSG_KSCrashField_Error, &crashContext->crash);
// Don't write the threads for user reported crashes to improve performance
if (crashContext->crash.threadTracingEnabled == true ||
crashContext->crash.crashType != BSG_KSCrashTypeUserReported) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above

@kattrali kattrali force-pushed the track-handled-unhandled branch from 262d6e5 to da26480 Compare September 29, 2017 17:05
Copy link
Contributor

@kattrali kattrali left a comment

Choose a reason for hiding this comment

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

🎉

@fractalwrench fractalwrench merged commit 14d586e into update-crash-reporter Oct 2, 2017
@fractalwrench fractalwrench deleted the track-handled-unhandled branch October 2, 2017 16:26
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