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

Update the severities value of BuildCheck results #10330

Merged

Conversation

f-alizada
Copy link
Contributor

@f-alizada f-alizada commented Jul 3, 2024

Fixes #10097

Context

Current severity model supports only Info, Warning, Error levels of the configurable values, and are not aligned with the documentation provided in the BuildCheck.
BuildCheck documentation currently provides the possibility to have the severity equal to None, Default, Suggestion

Changes Made

  • New confg values for severity are introduced:
    • None - will result the IsEnabled to be set to false
    • Default - if set to the rule, the default value of the rule will be used.
    • Info renamed to the Suggestion
  • Removed the possibility to configure the IsEnabled.
    • The property now is readOnly and will be determined based on the Severity value

Testing

Manual testing with additional automated test to cover the scenario of enablment of the rule

Copy link
Contributor

@maridematte maridematte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'd like to see somewhere in the documentation stating that to enable a rule or analyzer, the severity needs to be set. Just to make things clear for users.

@baronfel
Copy link
Member

baronfel commented Jul 3, 2024

@maridematte is that really the case? If no severity is set, then the 'default' severity for each rule should be used, which is variable and will lead to at least some rules being on by default without any user settings.

This is desired - generally when we make rules we want them to be on, and users have to do something extra to turn them off.

@f-alizada
Copy link
Contributor Author

Looks good, I'd like to see somewhere in the documentation stating that to enable a rule or analyzer, the severity needs to be set. Just to make things clear for users.

Thank you @maridematte, like @baronfel mentioned, it is not the case. and only enablment that should be happening is /analyze.
If there are no any .editorconfigs on the disk, then the default values are taken into the account. This topic contributed additional unit test case to cover specific scenario when no severity is added, and default is used

@baronfel baronfel changed the title Update the severities value of BuilcCheck results Update the severities value of BuildCheck results Jul 3, 2024
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick handling of this! It looks good - I'd only want this to be aligned with Roslyn (skipping the Silent severity)

@f-alizada
Copy link
Contributor Author

@JanKrivanek Thank you for the review.
Could you please clarify what you are suggesting is use only suggestion instead of the info (basically naming?)
My understanding was that to avoid the usage of something that is not supported yet. Because the suggestion is highlighetd in the IDE but the informational not, hence my thinking was to add it once we will have supported logs, Please let me know what do you think :) thanks

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

@f-alizada f-alizada merged commit 07be3a1 into dotnet:main Jul 4, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Severities model
4 participants