Skip to content
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

Add analysers that support our logging standards #33

Open
Audacia-RhysSmith opened this issue Aug 7, 2024 · 4 comments
Open

Add analysers that support our logging standards #33

Audacia-RhysSmith opened this issue Aug 7, 2024 · 4 comments

Comments

@Audacia-RhysSmith
Copy link
Contributor

Audacia-RhysSmith commented Aug 7, 2024

  1. If log messages contains an interpolated string, should use template message (FormatableString in C#)
  2. Every command or service injects ILogger
  3. Every command should have an LogInformation that starts with Entry and starts with Exit
  4. Every log message should have more 3 words? (thinking behind is that 3 words isnt descriptive enough)
  5. [Removed] Violation for using _logger.Log with signature of just string, there is another signature which has the log level passed which is fine
  6. Check for duplicate log statements
  7. Violation for if the amount of log statements if greater than the amount of "functionality" lines. If there was 5 lines in a method, should there be 8 lines of logging (thinking is this would violate clear and concise logging)
@richardb355
Copy link
Member

My two cents on each one:

  • There's already an MS analyzer that checks for this (CA2254); we could raise it to a warning in the default .editorconfig.
  • I like the idea, but we'd need a way to identify command/service classes, probably by being able to specify in configuration the base class/interface that all such classes should inherit from.
  • See above.
  • Maybe, but not totally convinced by this.
  • Don't all of the Log overloads take a LogLevel parameter?
  • Agreed.
  • Agreed, probably a low priority to implement though.

@jackpercy-acl
Copy link
Contributor

My input on each one too:

  • Agreed, but yes we should use CA2254. There is also CA2253 that I think we should add to the .editorconfig
  • I think this would be fine, we have an analyzer that Dtos must be record types by checking the filename. Could check for ending in Handler or Service in this case?
  • I think checking for entry/exit would be a bit harder to do, plus this can be achieved with cross-cutting concerns if using a command dispatcher/handler such as MediatR
  • Deciding if a log message is valuable/descriptive can be done at code review time IMO
  • As Richard said
  • Maybe check for duplicates
  • Agreed that this is probably not concise logging

@OwenLacey28
Copy link
Contributor

If log messages contains an interpolated string, should use template message (FormatableString in C#)

🟢 worth a very quick PR into Audacia.CodeAnalysis? Maybe at the same time as the logging standards are merged?

Every command or service injects ILogger

🟡 less convinced by this, do we want all of our commands/services to inject a logger?

Every log message should have more 3 words? (thinking behind is that 3 words isnt descriptive enough)

🟢 could be a char check - but agree with this in principle

Check for duplicate log statements

🟡 within the same method / class? For me this isn't a common enough issue, and doesn't catch scenarios like when copy/pasting from another area

Violation for if the amount of log statements if greater than the amount of "functionality" lines. If there was 5 lines in a method, should there be 8 lines of logging (thinking is this would violate clear and concise logging)

🟢 yeah why not

@Audacia-RhysSmith
Copy link
Contributor Author

Audacia-RhysSmith commented Aug 22, 2024

Created a PR to propose changes for points 1 and 2 - would appreciate any feedback

#36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants