Add accessibility modifiers#302
Conversation
| dotnet_code_quality.CA1852.ignore_internalsvisibleto = true | ||
|
|
||
| # Modifier preferences | ||
| dotnet_diagnostic.IDE0040.severity = warning # IDE0040: Add accessibility modifiers |
There was a problem hiding this comment.
There's a dotnet_style_require_accessibility_modifiers option for this higher up in the file. On the ipy project it's set to error. Guess we could also force csharp_preferred_modifier_order to be consistent...
There was a problem hiding this comment.
I'm not sure whether:
- You are calling out my use of
dotnet_diagnostic.IDE0040.severityrather thandotnet_style_require_accessibility_modifiers - The fact that I set it to
warningwhile Ipy sets it toerror
Ad 1: .editorconfig seems to be a mix of both so I had to choose one, but have no preference between them two (my only strong preference is not to use attributes for this purpose, if possible). So I am OK if we standardize on any form of the two used in .editorconfig.
Ad 2. Two things to add here: There is also a setting dotnet_analyzer_diagnostic.severity = warning in both the DLR and Ipy, which personally I think is a good choice. However, we have the setting TreatWarningsAsErrors so in practice there is no difference between settings severity to warning or error. I don't think TreatWarningsAsErrors is that desirable; we strive not to have any warnings anyway, and occasionally some warnings occur but should not break the build, e.g. it may take time or several steps to remove the warning, and in the meantime the project is not buildable. So I am in favour of removing TreatWarningsAsErrors and paying more attention to whether a rule is set to a warning or an error.
The second thing (assuming we abolish TreatWarningsAsErrors) is whether setting dotnet_style_require_accessibility_modifiers and csharp_preferred_modifier_order to error is the right choice. My general preference is to set style rules to warning (at most), not error. OTOH, a rule like "use is null iso == null" I would set to level error, since this is more than a readability style. I am surprised that those two rules on the Ipy side are set to error, while the default (which is a vast majority or rules) is set to warning. What made them so special?
There was a problem hiding this comment.
I guess the main point I was trying to make was that dotnet_style_require_accessibility_modifiers is already defined in the .editorconfig so we just have to change suggestion -> error (or warning) to effectively get the as this line? I'm not sure why they were specifically set to error as opposed to warning in ipy (probably my fault). Probably treating warnings as errors meant they were effectively the same so it didn't matter.
Regarding TreatWarningsAsErrors I guess the main advantage from my point of view was that I didn't have to dig through build logs to find warnings so I can nit pick on style (which I like to do). Though I guess since we didn't have EnforceCodeStyleInBuild it wasn't working anyway. 😄 That being said, I'm not opposed to dropping that restriction and lettings warnings be warnings again.
There was a problem hiding this comment.
Changing suggestion -> warning at dotnet_style_require_accessibility_modifiers higher up would make it applicable to both Src and Tests and my intention was to only clean up Src. Cleaning up Tests, while a noble goal by itself, is really very low on my list of interests; there is so much other stuff to focus on. Besides, I wanted to leave something for fellow nitpickers 😉
I will stick to using dotnet_diagnostic.IDE0040.severity at this place not to repeat the value of the option, which is set higher up (for_non_interface_members). In this way there is only one place in which analyzer's option value is being set. The same applies to csharp_preferred_modifier_order.
Regarding TreatWarningsAsErrors, I will open a separate issue where we can have a discussion on pros and cons, with the reasoning behind any decision saved for posterity. 😄 There is more to the story I'd like to say.
Readability improvement.
The default for accessibility of classes is
privateorinternal, depending on the context. Since the context is not always directly visible when reading the class definition (can be defined many lines away), it is desirable to be explicit.For fields, methods, properties, etc. it is always
private, but it's still good to have it stated explicitly too, for consistency. Such consistency is useful when searching the codebase using regex.