Fix issues found by Roslynator #1899
Conversation
| @@ -21,7 +21,7 @@ static class Padding | |||
| /// <summary> | |||
| /// Writes the provided value to the output, applying direction-based padding when <paramref name="alignment"/> is provided. | |||
| /// </summary> | |||
| public static void Apply(TextWriter output, string value, in Alignment? alignment) | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
Is making Alignment readonly a suitable alternative? (The in keyword was added here thanks to advice from a competing analyzer, IIRC 🤷 )
There was a problem hiding this comment.
Is making Alignment readonly a suitable alternative?
Alignment is already a readonly struct.
# Conflicts: # src/Serilog/Capturing/PropertyValueConverter.cs
|
my personal preference is to avoid roslyn analysers. been burned too many times with poor performance and bugs. when i do use them i only enable them for build. so disable realtime checking |
There was a problem hiding this comment.
I like some of the fixes/suggestions in here, but like @SimonCropp I'd lean towards keeping the solution lighter (and not having to potentially suppress another set of false-positives/unnecessary hints); Roslynator is installable as an IDE extension so perhaps that's enough, in this case?
| /// <summary> | ||
| /// Construct a <see cref="LoggingFailedException"/> to communicate a logging failure. | ||
| /// </summary> | ||
| public LoggingFailedException() |
There was a problem hiding this comment.
Default exception constructors usually set a default message.
There was a problem hiding this comment.
why does an exception need a default constructor?
There was a problem hiding this comment.
I've never really liked the guideline, but this is the source of the convention: https://learn.microsoft.com/en-us/dotnet/standard/exceptions/best-practices-for-exceptions#include-three-constructors-in-custom-exception-classes
| @@ -21,7 +21,7 @@ static class Padding | |||
| /// <summary> | |||
| /// Writes the provided value to the output, applying direction-based padding when <paramref name="alignment"/> is provided. | |||
| /// </summary> | |||
| public static void Apply(TextWriter output, string value, in Alignment? alignment) | |||
There was a problem hiding this comment.
Is making Alignment readonly a suitable alternative? (The in keyword was added here thanks to advice from a competing analyzer, IIRC 🤷 )
| @@ -830,7 +832,7 @@ namespace Serilog.Parsing | |||
| } | |||
| public sealed class PropertyToken : Serilog.Parsing.MessageTemplateToken | |||
| { | |||
| public PropertyToken(string propertyName, string rawText, string? format = null, in Serilog.Parsing.Alignment? alignment = null, Serilog.Parsing.Destructuring destructuring = 0) { } | |||
There was a problem hiding this comment.
Alignment is a readonly struct and works well with in but Nullable<Alignment> is not a readonly struct so in creates defensive copy as I understand.
The in keyword was added here thanks to advice from a competing analyzer, IIRC
No, I added in myself when making struct readonly.
|
rebased on dev |
|
rebased on dev |
|
Rebased on dev. @nblumhardt Anything else to do here? |
No description provided.