-
-
Notifications
You must be signed in to change notification settings - Fork 226
chore: Added DebugLoggerLevel to DebugLogger delegate #4457
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
Conversation
| /// <param name="message">The message string to write.</param> | ||
| /// <param name="args">Arguments for the formatted message string.</param> | ||
| public delegate void DebugLogger(string message, params object?[] args); | ||
| public delegate void DebugLogger(DebugLoggerLevel level, string message, params object?[] args); |
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.
question: breaking change
Since we don't mention Sentry.Android.AssemblyReader in the README as a top-level package,
nor in our documentation as a platform guide,
we do not consider changes to the public surface area of this Assembly a breaking change,
since it's main purpose is to be consumed by the Android TFMs of Sentry,
right?
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.
Hm, actually it's only used from here:
sentry-dotnet/src/Sentry/SentryOptions.cs
Lines 1196 to 1206 in eadf5f2
| /// <summary> | |
| /// Allows integrations to provide a custom assembly reader. | |
| /// </summary> | |
| /// <remarks> | |
| /// This is for Sentry use only, and can change without a major version bump. | |
| /// </remarks> | |
| #if !__MOBILE__ | |
| [CLSCompliant(false)] | |
| #endif | |
| [EditorBrowsable(EditorBrowsableState.Never)] | |
| public Func<string, PEReader?>? AssemblyReader { get; set; } |
It looks like Matt added that comment in #2127
@bruno-garcia given that sentry-xamarin is no longer being maintained, could we potentially move the AssemblyReader back into the main Sentry packages and make all this stuff internal again (not in this PR of course - probably for v6.0)?
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'm in favor of that, just get rid of that package and make our lifes easier
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.
to be fair, it should never have been created. It's very little code and only used in 2 package we own. We could have copy pasted things and manually keeped them in sync.
We knew Xamarin was schedule for retirement by then already.
Classic .NET overengineering
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.
Resolves #4382
#skip-changelog