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

Recommended quality profile #130

Merged
merged 13 commits into from Nov 25, 2018
Merged

Recommended quality profile #130

merged 13 commits into from Nov 25, 2018

Conversation

@mwz
Copy link
Owner

mwz commented Nov 23, 2018

It's a WIP, but I'll open for early feedback.

What it does is it sets up a recommended quality profile based on what was discussed in #80 including a list of exceptions, overridden rule severities and parameter values. I haven't thought about defining Scalastyle template instances yet and that's what missing in addition to tests.

Closes #80.

mwz added 3 commits Nov 23, 2018
Copy link
Contributor

BalmungSan left a comment

In general terms I like it.
The only thing that bother me is that we end up exposing too much.
If you recall, for the Scalastyle+Scapegoat quality profile I created some helper methods on both the Scalastyle & Scapegoat QualityProfiles to allow its creation without exposing the internal details.
I think we could do something similar, like a method activeAllRulesExcept(profile: Profile, predicate: String => Boolean) and we pass to that predicate the id of each rule - activeAllRules could be defined in terms of that method.

That would work for the exclusions, the problem comes with the overridden severities, parameters and templates... Maybe we could pass another function to process each rule not excluded - but I worry about complicating the solution too much, just by hiding a couple of internal details.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Nov 24, 2018

I started from a simple implementation, but what you're saying makes sense, especially given how we implemented the combined quality profile.

I've extracted the config with the blacklist and overrides into a separate case class, which I'm passing into the new activateWithOverrides function. It can be probably combined with the activateAllRules, so I'll probably refactor it a bit further. Also, things might change a bit when it comes to creating template instances, but I haven't worked out yet how we can do that.

)
}
// Enable Scapegoat rules excluding blacklisted rules. Overrides the severity.
ScapegoatQualityProfile.activateWithOverrides(profile, RecommendedQualityProfile.RuleOverrides)

// Ensure this is not the default profile.
profile.setDefault(false)

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 24, 2018

Contributor

I just noticed, this should be the default profile, right?
Or what do you think?, I mean for a fresh install this should be best option.
And, I'm sure that if one creates another profile which he/she wants to be the default it can be configured from the SonarQube web interface, and Sonar will override the settings - or at least I hope so.

This comment has been minimized.

Copy link
@mwz

mwz Nov 24, 2018

Author Owner

yeah I guess so - I'll try it out and see what happens

"null" -> Severity.MAJOR,
"var.field" -> Severity.MAJOR,
"var.local" -> Severity.MAJOR,
final val RuleOverrides: Overrides = Overrides(

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 24, 2018

Contributor

What about having two Overrides ?
One for Scalastyle and the other for Scapegoat

This comment has been minimized.

Copy link
@mwz

mwz Nov 24, 2018

Author Owner

yeah sure, we can split it up

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented Nov 24, 2018

"I've extracted the config with the blacklist and overrides into a separate case class, which I'm passing into the new activateWithOverrides function".

👍

"It can be probably combined with the activateAllRules, so I'll probably refactor it a bit further".

Yeah, it would be nice to reduce code duplication.

"Creating template instances, but I haven't worked out yet how we can do that".

I think I remember that they work very similar to override parameters.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Nov 24, 2018

I think I remember that they work very similar to override parameters.

Well, you can't just activate a template rule in a quality profile, you need to create a new rule from the template in the repository and then you can activate the new rule. So we have two options here:

  1. either set up the necessary template instances when we're creating all the other rules in the ScalastyleRulesRepository, or
  2. set up new rules from the RecommendedQualityProfile, which might require some server dependency injection magic - I'm trying to figure out how to look up the Scalastyle rules repository via sonar API, but I'm not sure if it's actually possible.

I think we should go for option 1 which makes the responsibilities of the quality profiles and rule repositories clear and simple, unless you think that we should explore option 2 or have some other suggestions.

@BalmungSan

This comment has been minimized.

Copy link
Contributor

BalmungSan commented Nov 24, 2018

I think we already provide a default instance of each template - thus, I think we only need to override their parameters.

@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Nov 24, 2018

Ok, cool 👍 I was thinking about those three templates which we haven't created instances for ("header.matches", "regex", "scaladoc"), but I don't think we would want to include those in the recommended quality profile anyway.
I'll go ahead with your suggestion then and add any other overrides that I think should be in the recommended profile.

mwz added 3 commits Nov 24, 2018
@@ -28,23 +28,22 @@ import org.sonar.api.server.profile.BuiltInQualityProfilesDefinition.NewBuiltInQ
/**
* Defines a quality profile recommended by sonar-scala (including Scalastyle and Scapegoat).
*/
final class RecommendedQualityProfile extends BuiltInQualityProfilesDefinition {
final class RecommendedQualityProfile(
scalastyleRulesRepository: ScalastyleRulesRepository

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 25, 2018

Contributor

Why do you need the ScalastyleRulesRepository ?
I don't see it anywhere in the code. Also, if you really need it, remember we need to inject it using DI.

overrides.severities
.get(inspection.id)
.foreach(severity => rule.overrideSeverity(severity.name))
overrides.foreach { overrides =>

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 25, 2018

Contributor

Maybe this would look better in a for?

for (overrides <- overrides; severity <- overrides.severities.get(inspection.id)) {
 rule.overrideSeverity(severity.name)
}
_.foreach {
case (k, v) => rule.overrideParam(k, v)
// Override rule params.
overrides.params

This comment has been minimized.

Copy link
@BalmungSan

BalmungSan Nov 25, 2018

Contributor

Maybe two nested fors in this case?

This comment has been minimized.

Copy link
@mwz

mwz Nov 25, 2018

Author Owner

I'm not sure if having nested for comprehensions would make it look any better. I originally had those as two separate flatMaps so I could go back to that if it makes it more readable for you?

mwz added 5 commits Nov 25, 2018
@mwz mwz changed the title WIP recommended quality profile Recommended quality profile Nov 25, 2018
@mwz

This comment has been minimized.

Copy link
Owner Author

mwz commented Nov 25, 2018

Ok, I've added unit tests, now I just need to check how making the quality profile a default works in practice.

Copy link
Contributor

BalmungSan left a comment

👍

@mwz mwz merged commit 60dedc3 into master Nov 25, 2018
1 check passed
1 check passed
ci/circleci: test Your tests passed on CircleCI!
Details
@mwz mwz deleted the recommended-quality-profile branch Nov 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.