-
Notifications
You must be signed in to change notification settings - Fork 0
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
fix: #9 Fix warnings #12
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working towards this tidy up. One thing that stands out however is the change of acronyms to PascalCasing. e.g. OS -> Os and IL to Il etc.
I've previously heard that PascalCasing for these is the Microsoft recommendation, however we've stuck with all caps for acronyms throughout our .NET code and so would like to stick with that for consistency please. I also personally like the readability of all caps better in most cases.
You may want to add in a dotsettings rule to avoid them showing up as warnings.
* ci: dotnet formatting * simplify job * test break formatting * fix format * dotnet format
Hi @QuantumNightmare I have applied your suggestions and restored the acronyms name formatting. |
Thanks, looks good 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just added some minor feedback
@@ -1,5 +1,8 @@ | |||
namespace Raygun.NetCore.Blazor.Server |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine we'll just want to delete this class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought the same, the Raygun.Blazor.Server
question is addressed here: #22
src/Raygun.NetCore.Blazor.WebAssembly/Extensions/WebAssemblyHostBuilderExtensions.cs
Outdated
Show resolved
Hide resolved
private string? _calculatedBrowserVersion; | ||
private string? _calculatedBrowserName; | ||
private string? _calculatedOSVersion; | ||
// private string? _browserManufacturer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should delete this unless we intend to wire it up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At one point I had intended to wire those up from the UserAgentHints, but then I discovered that the versioning at that level is a dictionary of multiple sets of values, including an extra key designed to throw off people doing things like this.
So I believe I decided to return the whole array (minus the BS extra key) instead, which means those can be deleted.
@@ -39,19 +39,19 @@ internal record EnvironmentDetails | |||
/// The company who manufactured the browser. | |||
/// </summary> | |||
[JsonPropertyName("browser")] | |||
public string BrowserManufacturer { get; set; } | |||
public string? BrowserManufacturer { get; set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside the scope of this PR, but in case it gets missed later, the JsonPropertyName meaning something different to the property name may be unintentional here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was intentional. I was looking for API consistency between what the browser returned, to help end users get a clear understanding of what the data point actually was. The JsonPropertyName
just mapped that clarity I was trying to provide at the code level to the value the Raygun API was expecting in the payload.
fix: #9 Fix warnings
Description 📝
Type of change
Updates
Screenshots 📷
Test plan 🧪
dotnet test
passes.Author to check 👓
Reviewer to check ✔️