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

[PS-97]: Extend RaygunEnvironmentMessage to add Maui specific information. #4

Merged
merged 23 commits into from
May 11, 2023

Conversation

Hamish-taylor
Copy link
Contributor

@Hamish-taylor Hamish-taylor commented May 8, 2023

The problem

The problem this pull request is trying to solve is that the Raygun4net environment message is limited and does not contain mobile specific fields that other Raygun mobile specific providers support. We would like to support these fields in the Maui provider. This pr also covers populating the fields with the appropriate data.

The implementation

There are two areas that were changed to enable this support, firstly a new RaygunMauiEnvironmentMessage class was created that extends the raygun4net.core RaygunEnvironmentMessage class. This new class contains the extra fields mentioned above. Secondly a RaygunClientTest (this name is only temporary) class was created extending RaygunClient. This class overrides the BuildMessage method, adding custom code to build the message with the new RaygunMauiEnvironmentMessage class.

Note
This PR proposes a approach to building the RaygunMessage that breaks from convention. Typically builder classes have been used to create and populate the message and its child classes. This is how it is done inside the Raygun4net.core provider.
My argument against this approach is that this creates unnecessary abstractions away from the data. Many of the builder methods just taking in a field as a parameter and store it inside of the underlying data structure, I propose eliminating the middle layer and instead to work with the data its self. In my approach the objects are explicitly created and populated, this makes it very clear what is going on and makes it very easy to modify in the future. With the builder approach, if you want to change one of the underlying datatypes, for example the RaygunEnvironmentMessage, you then have to change the RaygunEnvironmentMessageBuilder and the RaygunMessageBuilder. With this approach you simply have to change the EnvironmentMessage object you are creating and populate any extra fields. Given how small the code is to explicitly construct the objects without the builder I do not believe this is a concern that needs to be split of into its own classes. KISS

Author to check:

  • Builds pass
  • Tested in an alternative environment
  • Reviewed by another developer
  • Appropriate documentation written
  • Tested on Andoid
  • Tested on iOS
  • Tested on MacOS
  • Tested on Windows

Reviewer to check:

  • Change has been tested on Beta
  • Code is written to standards
  • Appropriate tests have been written

@Hamish-taylor Hamish-taylor changed the title Extend RaygunEnvironmentMessage to add Maui specific information. [PS-97]: Extend RaygunEnvironmentMessage to add Maui specific information. May 9, 2023
@Hamish-taylor Hamish-taylor requested a review from PanosNB May 9, 2023 04:14
Copy link
Collaborator

@PanosNB PanosNB left a comment

Choose a reason for hiding this comment

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

As discussed, also update the readme with platform-specific interpretations of total memory

README.md Outdated Show resolved Hide resolved
Raygun4Maui/RaygunMauiClient.cs Outdated Show resolved Hide resolved
Raygun4Maui/RaygunMauiClient.cs Show resolved Hide resolved
@PanosNB PanosNB self-requested a review May 10, 2023 23:05
Copy link
Collaborator

@PanosNB PanosNB left a comment

Choose a reason for hiding this comment

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

Great work!, just a couple of typos in the readme

README.md Outdated
@@ -131,7 +131,17 @@ try {

---
## Platform specific information
Raygun4Maui will automatically collect information specific to the environment the application is being run in. However, on iOS, Raygun4Maui cannot obtain the devices name. This is a privacy restriction put in place by Apple. If you would like this information to be collected and sent with crash reports you will have to [request for permission from apple](https://developer.apple.com/documentation/bundleresources/entitlements/com_apple_developer_device-information_user-assigned-device-name?language=objc).
Raygun4Maui will automatically collect information specific to the environment the application is being run in. However, there are inconsistency's between certain values across platforms.
Copy link
Collaborator

Choose a reason for hiding this comment

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

use plural: "inconsistencies"

README.md Outdated Show resolved Hide resolved
@Hamish-taylor Hamish-taylor merged commit e982a4f into master May 11, 2023
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.

3 participants