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

Add support for group rules #51

Closed
alrra opened this issue Mar 20, 2017 · 5 comments
Closed

Add support for group rules #51

alrra opened this issue Mar 20, 2017 · 5 comments

Comments

@alrra
Copy link
Contributor

alrra commented Mar 20, 2017

@molant:

Yes, we want to group them and I think it should be via the meta object. We can use the meta.docs.category or any another property we want. I don’t think extends is a good idea because that should be to extend a configuration set. We could add an option in .sonarrc to enable all rules within a category (that also works with the command line).

It could be something like:

“categories”: [“webapp”, “security”]

That can be mixed with the rules (rules will have higher priority so if we enable a category but disable a rule, all the rules for that category but that one will be enabled).

Problems for this approach:

  • How to set up the severity error (could be an object instead of just an array of strings)
  • How to set up the extended configuration (maybe it just enables the default values and then user needs to configure further via rules)
@molant
Copy link
Member

molant commented Apr 27, 2017

Another thing to keep in mind is that a rule might belong to several groups and could have different configurations in each one.
@alrra, can you put an example for that scenario?

@sarvaje
Copy link
Contributor

sarvaje commented May 15, 2017

@alrra thoughts about how this should work?

@alrra
Copy link
Contributor Author

alrra commented May 18, 2017

thoughts about how this should work?

I don't have a good idea yet. :(


They should be simple to enable, just like the regular rules. Maybe have them act the same as regular rules?

e.g. For a manifest group rule that contain all the manifest related rules:

"rules": {
    // ...
    "manifest": "warning",
    // ...
}

We could add an option in .sonarrc to enable all rules within a category (that also works with the command line).

Don't know is doing it automatically just based on the category is the best way.

Another thing to keep in mind is that a rule might belong to several groups and could have different configurations in each one.

One idea is to have them just like the other rules, but instead of calling create they can call another method with a JSON configuration similar to the content of the rules property from the .sonarrc file?

const groupRuleConfigs = {
  "rule1": {},
  "rule2": {
    "option1": "value",
    "option2": "value",
    // ...
  },
  // ...       
}

createGroupRule(groupRuleConfigs); 

Thoughts?

@molant
Copy link
Member

molant commented May 19, 2017

@alrra I was thinking more about the scenario you had in mind when you suggested this rule. What specific rule will need to be used in 2 groups with different configurations, and what would those configurations need to be?

@alrra
Copy link
Contributor Author

alrra commented May 19, 2017

What specific rule will need to be used in 2 groups with different configurations, and what would those configurations need to be?

Things are changing in the manifest space, but for now at least the display property will need to be standalone or fullscreen in order for some browsers to display the app install banner.

If we think of 2 categories, in the case of let's say (1) PWAs, the display property will be limited to those 2 values, but in the case of just having an ok, valid (2) manifest that restriction shouldn't exist.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants