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

Read issue severity from issue element rather than issue type element if present #307

Merged
merged 3 commits into from
Nov 2, 2021

Conversation

bdach
Copy link
Contributor

@bdach bdach commented Oct 27, 2021

Hi,

I am a contributor on a few projects, where we use NVika for parsing InspectCode reports. Today, we've encountered a situation wherein bumping one inspection's severity level to warning would show them as warnings in Rider, but the NVika parsed output would show them as suggestions.

The immediate cause of that, it seems, is that we have both an .editorconfig file, as well as a .DotSettings file, which happened to have conflicting severity levels. InspectCode handles that by giving precedence to .editorconfig, as well as attaching a Severity attribute to the issue at hand (rather than the issue type):

The XML comprises two parts: (...)

  • The list of found issues grouped by projects, where each issue has the following attributes: (...)
    • Severity — this attribute only appears if the severity of the issue differs from the severity of the corresponding inspection. This is possible if some projects within the solution have the 'Treat warnings as errors' option enabled and others do not - in this case some issues would have the 'Error' severity, which differs from the original 'Warning' severity.

(source for quotation)

This PR aims to follow that standard by reading and then giving precedence to the Severity attribute value if it is present on an <Issue> element, and only then falling back to the Severity read from the <IssueType> if the issue had no explicit severity.


I've attached a test case for this, but am unsure if it's packaged in the proper manner. Please advise if you would like to see adjustments to it. The code for the test is... a stretch, but I couldn't come up with a better illustrative example.

@laedit
Copy link
Owner

laedit commented Oct 27, 2021

Thanks a lot for your contribution!
I am so sorry, I am pretty busy in the next few days and I think I will not be able to review it before next week.

@bdach
Copy link
Contributor Author

bdach commented Oct 27, 2021

Absolutely, I understand. No rush necessary, we can get by in the meantime.

@@ -0,0 +1,11 @@
namespace InspectCodeTest
{
public class TestClass
Copy link
Owner

Choose a reason for hiding this comment

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

Could you rename the file and class to something more meaningful, maybe like Convert?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Have renamed to Conversions, hopefully that's a bit more fitting.

(Incidentally, when updating references, I've had a bit of trouble with the parser assuming that line endings are always 1 character, which is not the case on Windows. This is probably not a huge issue because most git repositories are likely to be configured to core.autocrlf correctly, and also out of scope of this pull, but I figure I may as well bring it up.)

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks for the heads-up!

@laedit
Copy link
Owner

laedit commented Nov 2, 2021

Thanks a lot for this fix!
I will try to release a new version before the end of the week.

@laedit laedit merged commit deb1fa8 into laedit:master Nov 2, 2021
laedit added a commit that referenced this pull request Nov 2, 2021
@bdach bdach deleted the inspection-severity-on-issue branch November 2, 2021 09:00
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.

2 participants