Skip to content

Conversation

@bitsandfoxes
Copy link
Contributor

@bitsandfoxes bitsandfoxes commented Nov 15, 2024

Reverts #1900

I had getsentry/sentry-dotnet#2715 and getsentry/sentry-dotnet#3630 wrong in my head and assumed the order was the issue.

#skip-changelog

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

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

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.

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 :)

@bruno-garcia bruno-garcia merged commit 3ea14cc into main Nov 15, 2024
94 checks passed
@bruno-garcia bruno-garcia deleted the fix/revert-logging-fix branch November 15, 2024 16:02
@bruno-garcia
Copy link
Member

"you missed a spot":

image

@RRode
Copy link

RRode commented Nov 20, 2024

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
Copy link
Contributor Author

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
Copy link
Member

bruno-garcia commented Nov 22, 2024

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

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