search

Suchiman / SerilogAnalyzer

Roslyn-based analysis for code using the Serilog logging library. Checks for common mistakes and usage problems.
Apache License 2.0
309 stars 29 forks source link

Serilog004 should come under a different category #63

Closed EddyHaigh closed 7 months ago

EddyHaigh commented 7 months ago

Currently Serilog004 is classed as a CodeQuality rule but within the explanation it comes across as a performance rule, would be better to simply change the current rule to become a performance rule or something other than code quality?

Line of code where category is situated

Suchiman commented 7 months ago

It is kind of both, violating that rule typically means that you're formatting log/event specific data into the template, which means that when you're using structured logging infrastructure, such as seq, you cannot "find all occurrences of this log", since each log has a unique "template" then. This makes it a quality rule first and foremost IMO, while the fact that fixing it also improves performance is secondary.

EddyHaigh commented 7 months ago

You make a good point. Its worth pointing out that the MS equivalent rule CA2254 it puts the rule under usage which makes a bit more sense and feels like a nice compromise.

Suchiman commented 7 months ago 
Usage rules:
Usage rules support proper usage of .NET.

Yeah that seems fair, question then is, currently all of the rules are classified as CodeQuality, but they all really are about using serilog properly. Would you change all of them?

EddyHaigh commented 7 months ago

From a quick gander at the rest of the rules

Suchiman commented 7 months ago

005 i would go for usage because it is a bug to use the same name twice and 006 is indeed naming (Naming rules support adherence to the naming conventions of the .NET design guidelines.)

I'll try to adjust them hopefully this evening, i have a ton of changes in my working copy that i'm scared of revisiting after letting them rest for so long but i can't run away from them forever.