Disable severity combobox for non-configurable rules

[Youssef1313]

Small part of #53171.

[mavasani]

I think this will be confusing. IMO we should show the descriptors but explicitly disable severity changes in the UI by greying out and disabling the combo box.

@jmarolf for his view.

[Youssef1313]

I think this will be confusing. IMO we should show the descriptors but explicitly disable severity changes in the UI by greying out and disabling the combo box.

@jmarolf for his view.

I thought the whole point of EditorConfig UI is to allow easily editing editorconfig files without having to remember IDs and option names/values or looking them up in the docs, etc.

I don't see the value of having a greyed out row. Why would I care about "NotConfigurable" rules if I'm using a feature made to configure rules?

To avoid the confusion, we might add a note at the top saying that it's a list of configurable rules.

[mavasani]

I thought the whole point of EditorConfig UI is to allow easily editing editorconfig files

The goal of the UX is to allow easy viewing, searching and editing all the applicable analyzers rules. Not seeing non-configurable rules in this UX would lead one to believe their analyzer package is missing these rules completely, which are listed in Analyzers node as well as analyzer package documentation. Additionally, it will be unclear why they are unable to configure or suppress a diagnostic for such a rule. A disabled entry in the UX with a hover message about non-configurability makes this explicit.

[Youssef1313]

Publishing build artifacts failed with an error: Not found PathtoPublish: D:\workspace_work\1\s\artifacts\bin\Microsoft.VisualStudio.LanguageServices.IntegrationTests\Debug\net472\xUnitResults

Closing and reopening.

[sharwell]

@Youssef1313 Can you rebase this onto release/dev16.11?

[jmarolf]

Thanks so much @Youssef1313 really like this change!

[jmarolf]

As @sharwell says, this should be retargets to the release/dev16.11

[sharwell]

Don't show descriptors with NotConfigurable tag in EditorConfig UI

Note that this is still the desirable behavior for rules that are both non-configurable and have default severity Hidden. The current form of this pull request fixes cases where the non-configurable rule has severity Info or higher, but we will need to go back and filter out the Hidden rules entirely.

Here's an example of another case where we filter a Hidden/Not-Configurable rule out of the UI entirely. Normally diagnostic IDs appear in the code fix preview, but for these cases they are omitted:

[mavasani]

we will need to go back and filter out the Hidden rules entirely.

Why? That seems very weird. What is the harm in showing these hidden non-configurable rules in UX?

[sharwell]

Why? That seems very weird. What is the harm in showing these hidden non-configurable rules in UX?

All such cases I'm aware of, including the two cases in #53171, two additional cases from Roslyn in the past, and cases in StyleCop Analyzers, are descriptors created with the direct intent to hide the diagnostic from the UI. The diagnostics are an implementation detail created only to support code fixes that act like refactorings, and like refactorings, they are designed to not appear as diagnostics.

The rule set editor shows the correct behavior here:

[Youssef1313]

I rebased into dev16.11 branch.

I'm not against any of the two approaches, however, the approach I prefer more is to not show them at all ( what I initially had in 17a8090). @mavasani @sharwell @jmarolf I'd be happy to revert back to the first approach or keep the PR as-is once you discuss this and decide the way it should be.

[mavasani]

I'd be happy to revert back to the first approach

Please don't - as I stated above, searching applicable analyzers is one of the biggest feature requirements for this UX. It is not just about editing. Ruleset editor was mostly popular because of the ability to search all applicable rules - editing the values can easily be done manually as well once you get used to the syntax.

[mavasani]

descriptors created with the direct intent to hide the diagnostic from the UI

These analyzers are wrongly authored. If you want a non-configurable hidden diagnostic, it should be authored as a refactoring. If you are doing so just to change the applicable span for code fix, again the issue here is our system does not support specifying this additional metadata about diagnostics/code fix presentation. If you are doing this to get a FixAll experience, this is primarily due to lack of FixAll support for refactoring, a known, pending feature request.

I stand by my view that a P0 for this UX is ability to search for every single applicable analyzer rule ID. IMO, its even more important feature then editing the severity/code style values. Adding filters such as hiding non-configurable hidden diagnostics, may be fine as a short term workaround (though I am still not sure if it is required). It is absolutely not the right long term solution.

[sharwell]

Ruleset editor was mostly popular because of the ability to search all applicable rules ...

The rule set editor could not search all applicable rules. It followed the rule I have been requesting: all rules except ones which are both non-configurable and have hidden severity.

These analyzers are wrongly authored.

The analyzers are authored according to patterns established in Visual Studio 2015 (Roslyn 1.x) and still supported. Other parts of the UI established and maintained precedent that authors can reliably expect these diagnostics to not appear in the UI. There are two places, both in newer UIs, where these currently appear but need to be removed.

Adding filters such as hiding non-configurable hidden diagnostics, may be fine as a short term workaround (though I am still not sure if it is required). It is absolutely not the right long term solution.

It is required and expected. Third-party analyzers have been designed and implemented according to this well-established pattern, and there is no open request for it to change as it relates to any feature. For all examples provided so far, there is no value in showing the diagnostic. If this behavior is not blocking any feature development, why would we care to change it?

Put another way: why would a user want to see/search for IDE0005_gen? Even if it shows up in this list, it won't show up anywhere else. Even if you open a file where the diagnostic is reported there is no visible indication of it.

[JoeRobich]

Changing target branch to 16.11 servicing branch

[jmarolf]

retargeting to dev17 as this missed dev16.11

[jmarolf]

@Youssef1313 unless you have an objections I plan to merge this.

[Youssef1313]

@Youssef1313 unless you have an objections I plan to merge this.

I don't. Let's get it in 🎉