feat: Enable Rule CA2211 #9857
feat: Enable Rule CA2211 #9857
Conversation
|
@robloo Can you review? |
|
You can test this PR using the following package version. |
| @@ -31,7 +31,7 @@ public DataGridCheckBoxColumn() | |||
| /// <summary> | |||
| /// Defines the <see cref="IsThreeState"/> property. | |||
| /// </summary> | |||
| public static StyledProperty<bool> IsThreeStateProperty = | |||
| public readonly static StyledProperty<bool> IsThreeStateProperty = | |||
There was a problem hiding this comment.
nit: Convention in this project seems to be "static readonly" ordering
| @@ -8,6 +8,6 @@ public static class ToolTipDiagnostics | |||
| /// <summary> | |||
| /// Provides access to the internal <see cref="ToolTip.ToolTipProperty"/> for use in DevTools. | |||
| /// </summary> | |||
| public static AvaloniaProperty<ToolTip?> ToolTipProperty = ToolTip.ToolTipProperty; | |||
| public readonly static AvaloniaProperty<ToolTip?> ToolTipProperty = ToolTip.ToolTipProperty; | |||
There was a problem hiding this comment.
nit: Convention in this project seems to be "static readonly" ordering
| #pragma warning disable CA2211 // Non-constant fields should not be visible | ||
| protected static Color[,]? _colorChart = null; | ||
| protected static object _colorChartMutex = new object(); | ||
| #pragma warning restore CA2211 // Non-constant fields should not be visible | ||
| protected static readonly object _colorChartMutex = new(); |
There was a problem hiding this comment.
My comments here apply to all 4 affect color palettes.
- Firstly I'm not a fan of rule CA2211 for exactly this reason. It doesn't allow for protected fields which absolutely have a usage and don't violate the non-public principle. The rule needs to be fixed...
- With that said, the chart itself was made protected before the final design was complete. There was a thought that it would be needed in derived types. However, with the colors now held in an enum (compiled in for performance) and then the inability to share color charts between half/full palettes, I think this design is obsolete. My suggestion is to make both of these fields
private. - I don't care about the
new()syntax usage. This is still largely developer preference so I probably wouldn't have changed it but it doesn't matter. Avalonia seems to prefer the shortest possible code.
private static Color[,]? _colorChart = null;
private static object _colorChartMutex = new();
private void InitColorChart()If this ever needs to be undone in the future (I doubt it) It is very easy and non-breaking to make them protected again.
|
You can test this PR using the following package version. |
|
You can test this PR using the following package version. |
What does the pull request do?
CA2211
What is the current behavior?
What is the updated/expected behavior with this PR?
How was the solution implemented (if it's not obvious)?
Checklist
Breaking changes
Obsoletions / Deprecations
Fixed issues
Part of #8944