search

getsentry / sentry-unity

Development of Sentry SDK for Unity
https://docs.sentry.io/platforms/unity/
MIT License
218 stars 52 forks source link

fix: Logger argument order #1901

Closed bitsandfoxes closed 1 week ago

bitsandfoxes commented 1 week ago

Reverts https://github.com/getsentry/sentry-unity/pull/1900

I had https://github.com/getsentry/sentry-dotnet/pull/2715 and https://github.com/getsentry/sentry-dotnet/pull/3630 wrong in my head and assumed the order was the issue.

skip-changelog

bruno-garcia commented 1 week ago

"you missed a spot":

image

RRode commented 5 days ago

we really need an analyzer for this. It's the same logic as string.Format so there's for sure analyzers out there that can do this, so it would be a copy of it.

@bruno-garcia I also checked, but couldn't find anything for ILogger or in your case IDiagnosticLogger

We got an analyzer in the dotnet repo (and the IDiagnosticLogger interface is also there): getsentry/sentry-dotnet#3725

The specialist is @RRode who might be interested in this :)

Can't promise anything, but will give it a try. If I understand the issue, you would want to prevent wrong method overload usage right? The question here is more what makes sense to allow. IMO it never makes sense to provide an exception as an string format param / args, since the default ToString doesn't provide much of a logging value. Also providing exception properties under params seems somewhat off, since the exception overload should log all relevant exception information. So is it fine to raise a warning if an exception is found under params?

bitsandfoxes commented 4 days ago

So is it fine to raise a warning if an exception is found under params?

Ideally, it'd be a bit more "in your face" than just a warning.

bruno-garcia commented 4 days ago

We have warnings as errors, so that would be fine. It would actually break the build if we made a mistake