Skip to content

[CR-204] Have the SendingMessage event include the original exception #434

Open
mduncan26 wants to merge 20 commits intomasterfrom
mb/cr-204/sendingmessage-original-exception
Open

[CR-204] Have the SendingMessage event include the original exception #434
mduncan26 wants to merge 20 commits intomasterfrom
mb/cr-204/sendingmessage-original-exception

Conversation

@mduncan26
Copy link
Contributor

@mduncan26 mduncan26 commented Jul 17, 2020

Update the SendingMessage event to include the original exception alongside the RaygunMessage object. This allows customers to perform further operations on the exception to extract contextual information only available on the exception.

Ticket: https://raygun.atlassian.net/browse/CR-204

MattByers and others added 19 commits July 15, 2020 11:24
…entArgs. Updated Raygun4Net4 to pass through original exception.
Ensure API backwards compatibility
Formatting
Ensure API backwards compatibility
Ensure API backwards compatibility
Updating WinRT to allow exception obj during SendingMessage event
Must declare properties to build WindowsStore
Fix for Mindscape.Raygun4Net.NetCoreproject failing to load
Bump MVC provider version to 5.11.0
Give Raygun4Net.Core its own AssemblyVersion file.
Bump Raygun4Net.Core package to version 5.11.0
Bump raygun4net.core dependency version to 5.11.0
Bump versions to 5.11.0
Bump raygun4net provider version to 5.11.0
Separating Xamarin iOS from changes.
Separating Xamarin Android from changes.
Separating Xamarin Mac from changes.
samuel-holt
samuel-holt previously approved these changes Jul 21, 2020
Copy link
Contributor

@samuel-holt samuel-holt left a comment

Choose a reason for hiding this comment

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

👍 This change seems straightforward, nice work

MattByers
MattByers previously approved these changes Jul 21, 2020
Copy link
Contributor

@QuantumNightmare QuantumNightmare left a comment

Choose a reason for hiding this comment

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

I have pointed out some methods that now require the new Exception parameter as they don't specify a default value, and I believe this would cause a breaking change.

I'm also going to point out this somewhat related blog post around being careful when using optional parameters. It suggests that adding optional parameters to an existing method can cause runtime exceptions if an application has not been compiled, but has a new version of a client library that has added optional parameters to existing methods. I would expect applications to typically be compiled when upgrading to newer versions of client libraries though, so maybe it's not a concern for us.

}

/// <summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment looks incorrect and could be deleted


// Returns true if the message can be sent, false if the sending is canceled.
protected bool OnSendingMessage(RaygunMessage raygunMessage)
protected bool OnSendingMessage(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to give the exception parameter a default value of null here in case someone has an extension to this class and is calling this method?


// Returns true if the message can be sent, false if the sending is canceled.
protected bool OnSendingMessage(RaygunMessage raygunMessage)
protected bool OnSendingMessage(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to give the exception parameter a default value of null here in case someone has an extension to this class and is calling this method?

/// <param name="raygunMessage">The RaygunMessage to send. This needs its OccurredOn property
/// set to a valid DateTime and as much of the Details property as is available.</param>
public async Task SendAsync(RaygunMessage raygunMessage)
public async Task SendAsync(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the new exception parameter doesn't have a default value, couldn't this cause a breaking change?

/// <param name="raygunMessage">The RaygunMessage to send. This needs its OccurredOn property
/// set to a valid DateTime and as much of the Details property as is available.</param>
public void Send(RaygunMessage raygunMessage)
public void Send(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the new exception parameter doesn't have a default value, couldn't this cause a breaking change?

/// set to a valid DateTime and as much of the Details property as is available.</param>
public void SendInBackground(RaygunMessage raygunMessage)
/// <param name="exception">The original exception that generated the RaygunMessage</param>
public void SendInBackground(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the new exception parameter doesn't have a default value, couldn't this cause a breaking change?


// Returns true if the message can be sent, false if the sending is canceled.
protected bool OnSendingMessage(RaygunMessage raygunMessage)
protected bool OnSendingMessage(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to give the exception parameter a default value of null here in case someone has an extension to this class and is calling this method?

private RaygunMessage _raygunMessage;
private Exception _exception;

public RaygunSendingMessageEventArgs(RaygunMessage message, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unlikely, but do we want to give the new exception parameter a null default, or keep in the old constructor as an overload - just in case someone is using the old constructor in which case this will be a breaking change.

/// set to a valid DateTime and as much of the Details property as is available.</param>
public void SendInBackground(RaygunMessage raygunMessage)
/// <param name="exception">The original exception that generated the RaygunMessage</param>
public void SendInBackground(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the new exception parameter doesn't have a default value, couldn't this cause a breaking change?

/// set to a valid DateTime and as much of the Details property as is available.</param>
public void SendInBackground(RaygunMessage raygunMessage)
/// <param name="exception">The original exception that generated the RaygunMessage</param>
public void SendInBackground(RaygunMessage raygunMessage, Exception exception)
Copy link
Contributor

Choose a reason for hiding this comment

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

If the new exception parameter doesn't have a default value, couldn't this cause a breaking change?

Addressing PR comments
@krishnaKapadia krishnaKapadia dismissed stale reviews from MattByers and samuel-holt via 1661987 August 30, 2021 21:00
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.

4 participants