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

Deprecate 'severity' field for IDE code style editorconfig syntax #44201

Closed
mavasani opened this issue May 13, 2020 · 18 comments
Closed

Deprecate 'severity' field for IDE code style editorconfig syntax #44201

mavasani opened this issue May 13, 2020 · 18 comments
Assignees
Labels
Area-IDE Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality IDE-CodeStyle Built-in analyzers, fixes, and refactorings New Feature - Editor Config
Milestone

Comments

@mavasani
Copy link
Contributor

Current IDE code style options support the following editorconfig syntax:

option_name = value:severity

For example:

[*.cs]
dotnet_style_qualification_for_field = true:warning

Command line compiler supports configuring severity of a specific diagnostic ID with the following editorconfig syntax:

dotnet_diagnostic.RuleID.severity = severity

This former syntax (option_name = value:severity) allows configuring both code style option value and severity. However, the severity setting from this syntax is only respected when executing the corresponding analyzer in the IDE live analysis. This syntax is not recognized by the command line compiler and/or the analyzer driver, so the severity setting above is redundant when executing from an explicit command line build. Additionally, with the command line compiler optimization implemented in #43546, we no longer execute hidden/suggestion analyzers in command line builds, unless the user has explicitly specified the analyzer diagnostic ID as a warning or error with the dotnet_diagnostic.RuleID.severity = severity syntax. This essentially means that the only way to enforce an IDE code style rule on command line build from our CodeStyle NuGet package is by using the latter compiler recognized syntax. Given this behavior, we should deprecate the former syntax and only allow option_name = value in the code style option syntax, and require the latter syntax for severity configuration.

@mavasani mavasani added this to the 16.7.P2 milestone May 13, 2020
@mavasani mavasani self-assigned this May 13, 2020
@mavasani
Copy link
Contributor Author

Tag @mikadumont @sharwell

mavasani added a commit to mavasani/roslyn that referenced this issue May 13, 2020
… IDE projects

My prior attempt to enable this as a build warning in dotnet#44164 did not work due to dotnet#44201. This PR actually adds the build enforcement and fixes the violations.
@sharwell sharwell added Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality Need Design Review The end user experience design needs to be reviewed and approved. and removed Bug labels May 18, 2020
@JoeRobich
Copy link
Member

Design Review

  • No object to the proposal
  • Suggestion to enhance MadsK extension to generate the new form and have code fix to migrate existing configuration to the new form.

@JoeRobich JoeRobich removed the Need Design Review The end user experience design needs to be reviewed and approved. label May 18, 2020
@jinujoseph jinujoseph modified the milestones: 16.7.P2, 16.7 Jun 9, 2020
@huoyaoyuan
Copy link
Member

I'm a bit confused with this change:

There's no documentation about diagnostic ids and corresponding rules now.

Even if there is one, splitting rule preference and severity into two places is not friendly.

I cannot work properly now without MadsK's extension. Please consider to enhance first class IDE support for editorconfig.

Ideally, I want a similar experience with IntelliSense of project files: it shows available diagnostic ids and their explanations.

Further more, you may create a GUI of it (reusing the parts from VS options). JetBrains Rider is already doing so.

@mavasani
Copy link
Contributor Author

Tagging @mikadumont - yes, we understand the concerns, but allowing controlling severity for each option, and then mapping it to severities of specific diagnostic ID(s) is difficult to implement in the compiler.

As you mentioned, I think the best option is to provide a GUI for editorconfig to allow users to get the experience of controlling severity for each code style without having to be affected by how these are represented in the underlying editorconfig, which can still have separate entries for severity and code style value.

@mavasani mavasani added the IDE-CodeStyle Built-in analyzers, fixes, and refactorings label Jul 15, 2020
@RehanSaeed
Copy link

Have I understood correctly that the proposed solution is to go with a separate setting for the severity, while using a rule ID to refer to it? The rule ID is opaque and requires either manual lookup in documentation or a UI to do the lookup for you. That seems to be a regression in usability and authoring of .editorconfig files. It also makes the .editorconfig a lot more verbose and less terse.

Current Solution

[*.cs]
dotnet_style_qualification_for_field = true:warning

Proposed Solution

[*.cs]
dotnet_style_qualification_for_field = true
dotnet_diagnostic.RuleID.severity = warning

@mavasani
Copy link
Contributor Author

@RehanSaeed The proposed solution is:

  1. Accept the fact that it is not possible or scalable for any person to remember either the code style names, IDE diagnostic rule IDs and/or the exact .editorconfig syntax for code style or severity configuration entries.
  2. Invest in an Editorconfig UI for users to do any style or severity configuration settings, on the same lines as the UI available in Tools/Options for VS wide per-user code style settings.

@mavasani
Copy link
Contributor Author

Actually, I think we might have couple of ways to get around not deprecating the code style syntax for severity setting:

  1. Switch all the rules in the CodeStyle NuGet package to be warnings by default OR
  2. Recommend users who do not want to use dotnet_diagnostic.RuleId.severity based severity configuration to follow the suggestion at editorconfig c# language rules doesn't trigger build failure #33558 (comment) to default all rules in the package to be warnings by default.

In both these cases, all rules get turned on for build as warnings and once that is done, both the syntaxes for severity setting should work fine to change the severity or disable rules with subsequent editorconfig entries.

@mavasani
Copy link
Contributor Author

https://developercommunity.visualstudio.com/content/problem/1116079/editorconfig-improvement.html shows an example for a user who contradicts the claim made by @RehanSaeed that configuring the severity of code style options by code style value name is easier to remember as compared to using diagnostic IDs. They prefer the other way.

I think this shows that @jmarolf's work towards a UI to help users view and configure styles and severities without worrying about the underlying syntax is extremely important.

@jinujoseph jinujoseph modified the milestones: 16.7, Backlog Jul 26, 2020
mavasani added a commit to mavasani/roslyn that referenced this issue Oct 14, 2020
Contributes towards dotnet#44201

- All code style option editorconfig entries can now be specified as `option_name = option_value` without `:severity` suffix
- `Configure code style option` code fix will no longer add add `:severity` suffix for new entries. For fix that changes existing entries which have `:severity` suffix, it will retain the existing severity suffix. Note that `Configure severity` code fix was already switched to use `dotnet_diagnostic.RuleID.severity` entries for code style diagnostics in dotnet#47052.

Once we have the editorconfig UX to change code style option value and severity, we can safely deprecate the `:severity` suffix in option value syntax. All our UI gestures will use the `option_name = option_value` syntax for code style value changes and `dotnet_diagnostic.RuleID.severity = severity` syntax for severity changes and users do not need to deal with these manually editing or understanind these different syntaxes or mapping rule IDs to option names and vice versa for code styles.
@RehanSaeed
Copy link

Even though I have set dotnet_analyzer_diagnostic.severity = warning in VS 16.8, I seem to have lost some warnings which I can only get back by setting a specific severity using the old style:

csharp_using_directive_placement = inside_namespace:warning

Am I correct in thinking that I need to wait for VS 16.9 to completely switch over?

@maxild
Copy link

maxild commented Jan 16, 2021

I can confirm that using vs2019 16.8.4 with sdk 5.0.102 the (soon...but how soon?) deprecated syntax often is necessary to see warnings (and avoid analyzer exceptions, in some rare cases: Microsoft.CodeAnalysis.CSharp.Diagnostics.AddBraces.CSharpAddBracesDiagnosticAnalyzer threw an exception of type System.ArgumentNullException with message 'Value cannot be null):

csharp_prefer_braces = when_multiline:warning

So when should the syntax be switched over?

@mavasani
Copy link
Contributor Author

@maxild Starting 16.9, all code style options can be written without severity suffix.

@RehanSaeed
Copy link

For backward compatability I have been using both forms for setting severity. What version of VS will the older [rule] = [value]:[severity] syntax no longer work? When do you recommend to make the switch?

@bording
Copy link

bording commented Mar 2, 2021

@maxild Starting 16.9, all code style options can be written without severity suffix.

@mavasani With 16.9 being released today, I have tried to use the simpler syntax, and it appears that it does not work. Did this change not make it into 16.9 after all?

@patricksadowski
Copy link

patricksadowski commented Mar 31, 2021

IDE1006 takes care of all naming rules. Unfortunately you have to specify all properties (including severity) of a naming rule to activate a rule. You are now forced to set severity for each naming rule to default to get the configured behavior of IDE1006. This confused me, but now it's working.

@vikekh
Copy link

vikekh commented Dec 1, 2022

How will this work when multiple code style options map to 1 rule? E.g. if I have:

csharp_style_var_for_built_in_types = false:suggestion
csharp_style_var_when_type_is_apparent = false:none
csharp_style_var_elsewhere = false:warning

My understanding is that false here maps to IDE0008 ("Use explicit type instead of var") but with setting severity on the rule I can no longer have different severity based on the conditions that the code style options was targeting?

@cremor
Copy link

cremor commented Dec 1, 2022

Good question @vikekh! I really hope that we won't loose those separate severity settings.

@RehanSaeed
Copy link

@mavasani What is the latest on this? Which version of VS can we expect to use the new syntax? Also, how will this #44201 (comment) work?

@mavasani
Copy link
Contributor Author

mavasani commented Jan 3, 2024

Given the customer feedback, we decided to not deprecate the :severity syntax, but instead respect it now. See #52991.

Closing this issue as Won't fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-IDE Concept-Design Debt Engineering Debt, Design Debt, or poor product code quality IDE-CodeStyle Built-in analyzers, fixes, and refactorings New Feature - Editor Config
Projects
Status: Complete
Development

No branches or pull requests