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

Severity level for rules #533

Closed
schalkms opened this issue Nov 5, 2017 · 12 comments
Closed

Severity level for rules #533

schalkms opened this issue Nov 5, 2017 · 12 comments

Comments

@schalkms
Copy link
Member

schalkms commented Nov 5, 2017

At the moment detekt has severity grades. Additionally, severity levels might be useful.
Some rule violation are not so tragic than others.
A detekt configuration option to only report issues with severity level < config might also be useful.

@Mauin
Copy link
Collaborator

Mauin commented Nov 13, 2017

Similar to this, should it be possible to override the severity of an issue in the config? Say I don't consider catching too generic exceptions as an ERROR but just a WARNING, I could define that in the config?

@schalkms
Copy link
Member Author

Good idea!

@rvoortman
Copy link
Contributor

I want to add that detekt-cli should also print the severity of the issue; this way I'm able to catch the severity and integrate it with Phabricator (https://github.com/moneybird/arc-detekt-linter).

@arturbosch
Copy link
Member

Hey, interesting project :).
Have tried running detekt with --output /your/dir --output-name filename and parsing the xml file?
We use the chekstyle xml format.

@rvoortman
Copy link
Contributor

@arturbosch not yet; since we needed the project pretty fast and we already had a regex to grab the output, it was easier to get it this way.

That said: I'm certainly going to implement this in the future, since it provides more information.

@rvoortman
Copy link
Contributor

Off topic: @arturbosch I've added severity for the arcanist detekt linter; we're reviewing the code and functionality, but it should be online by the end of the week.

On topic: We're now facing similar issues in that we find certain errors less problematic then detekt thinks.

@arturbosch
Copy link
Member

That's where the configuration file shines. Just turn off rules you dont like or change some thresholds.
Also make sure to make use of the baseline feature if needed ^^

@arturbosch
Copy link
Member

Ok, I have reread the issue description. Kinda makes sense.
But how should these severity levels look like? Adding an extra property for this into the code is kinda meh What do you think?

@rvoortman
Copy link
Contributor

rvoortman commented Feb 22, 2018

Sure thing, but for example a 'MagicNumber' is something we consider as an advice, since they are not always a warning. Because it is a warning, phabricator will fail the build, which could result in problems if we're not careful. However, I still want detekt to show me this error, since it could be that we've missed it.

We can not override the severity in the config file (as far as I'm aware). I think it would be great to just have an option in the configuration that allows me to override the severity; if it's not set, we can just use the default severity.

EDIT: I've not yet dived enough in the code of detekt to give a clear opinion on how to implement this. How easy is it to pass another configuration option to the rules?

@arturbosch
Copy link
Member

arturbosch commented Feb 22, 2018

Having an option to change the severity within the configuration file is a good idea (also for suppress aliases).
Just did a fast code check (ConfigAware, DefaultContext, Rule). Accessing random configuration properties within the rule is a no brainer (valueOrDefault). Unfortunately changing Issue.severity from a generic point like Rule.init or DefaultContext.report is undoable ... need to investigate more

Using preVisit is kinda meh:

	override fun preVisit(root: KtFile) {
		issue.severity = valueOrDefault("severity", issue.severity)
	}

@arturbosch
Copy link
Member

I'm assuming your parsing the xml output? It is Checkstyle-based, so the severity is translated to checkstyle's style

	private val Finding.messageType: MessageType
		get() = when (issue.severity) {
			Severity.CodeSmell -> MessageType.Warning()
			Severity.Style -> MessageType.Warning()
			Severity.Warning -> MessageType.Warning()
			Severity.Maintainability -> MessageType.Warning()
			Severity.Defect -> MessageType.Error()
			Severity.Minor -> MessageType.Info()
			Severity.Security -> MessageType.Fatal()
			Severity.Performance -> MessageType.Warning()
		}

One generic way to achieve changing the reported severity could be to implement a new output report format.
or enhance the checkstyle format. (see OutputReport and Extension classes)

@chao2zhang
Copy link
Member

Progress will be tracked in #3274

Roadmap automation moved this from Backlog to Done Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Roadmap
  
Done
Development

No branches or pull requests

5 participants