Skip to content
This repository has been archived by the owner on Jul 8, 2019. It is now read-only.

Support for mapping custom TSLint rules to dedicated Sonar rules #31

Merged
merged 10 commits into from
Jul 23, 2016

Conversation

drywolf
Copy link
Contributor

@drywolf drywolf commented Apr 17, 2016

I performed the following additions & changes:

  • moved the definitions of core TSLint rules out of TsRulesDefinition.java into a separate .properties file (for better maintainability & extensibility, as well as a common way to handle core TSLint rules and custom TSLint rules within Sonar)
  • added methods to parse Sonar rule definitions from a configuration string (structured as in a Java properties definition file)
  • added a new configuration property 'sonar.ts.tslint.customrules' (available globally for the plugin and also individually for projects)
  • added some tests for some of the new functionality, updated some tests to reflect the changes to the existing code

- loading core rules from property file
- added missing rules to tests
- added test for unexpected rules in plugin
added configuration property to define custom TSLint rules for sonar analysis
- added documentation for the 'sonar.ts.tslint.customrules' property
- added a test for disabled TSLint rules in the configuration
@coveralls
Copy link

Coverage Status

Coverage decreased (-4.4%) to 72.727% when pulling dac34d9 on drywolf:master into d1a6faf on Pablissimo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.8%) to 73.278% when pulling d47cd8d on drywolf:master into d1a6faf on Pablissimo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.3%) to 73.829% when pulling a6d1d90 on drywolf:master into d1a6faf on Pablissimo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-3.3%) to 73.829% when pulling 2194422 on drywolf:master into d1a6faf on Pablissimo:master.

@Pablissimo
Copy link
Owner

Sorry I've not had time to review and merge this yet, working on finding some!

@Pablissimo
Copy link
Owner

Took a day off work, so finally have time to look at this! Sorry for delay:

  • Not sure that the sonar.ts.customrules property can be project-level defined, as you could end up advertising rules via project analysis that the server doesn't know about which would cause it to fail - will test it but suspect it needs to be global-only
  • I think I'd like to rename the setting so that it's clearer what it's about, but can't think of anything just yet - I'm wondering whether even calling them 'custom' rules is useful, or if instead (if they're global) we can do it convention-based and just look at a collection of configuration files in some location and parse them all. That way you can have multiple custom rulesets all loaded (so long as there are no naming conflicts, though we can use the filename or part of it as part of the rulename when it's loaded)
  • You've gone with underscore_casing rather than camelCasing in most of your code which I'd rather see reverted so that it's consistent with the rest of the plugin

I think there're pretty good functional wins though, loading in new rules via configuration rather than needing a plugin update seems like a pretty solid victory!

@drywolf
Copy link
Contributor Author

drywolf commented May 31, 2016

Thanks for the Review, I agree on those points.
I will probably not have much time to work on the PR for a bit, I will let you know once I'm starting work on those adjustments.

# Conflicts:
#	src/main/java/com/pablissimo/sonar/TsLintSensor.java
#	src/test/java/com/pablissimo/sonar/TsLintSensorTest.java
@drywolf
Copy link
Contributor Author

drywolf commented Jul 2, 2016

Hi @Pablissimo

I have started work on the discussed changes to the PR and saw that there is another PR #37 that has some changes that would need to be reworked to be compatible with this one.
What are your thoughts on how we should handle the two PRs ... I would also be willing to merge the changes of #37 after the more fundamental changes of this PR have been integrated into master.

Just let me know how you think we should handle it.

Regards

@coveralls
Copy link

Coverage Status

Coverage decreased (-2.9%) to 74.936% when pulling f17b84b on drywolf:master into dc44ea1 on Pablissimo:master.

@Pablissimo
Copy link
Owner

I agree - I'd take your changes to master first then I can rework @NikitaEgorov's changes since they're pretty much incompatible with the change to how we'll then be storing rules. To be honest, it shouldn't be a big job. Means I can't merge their Nikita's changes as a PR directly but I can use the work that went into getting the right tech debt values!

@drywolf
Copy link
Contributor Author

drywolf commented Jul 5, 2016

I'm currently looking into making the transition to support multiple rule configuration collections.

This is what I have currently ... instead of a single text config parameter in the plugin settings, we can now specify multiple configurations. That way it should be easier to manage multiple TsLint-Sonar-Rules configurations for multiple corresponding TsLint rules packages ...

image

For demonstration I added three separate configurations there which would logically map to separate TsLint rule packages (they will just be globally concatenated in Sonar, the separation is just for the user's convenience)

The rules will then be available in the TsLint profile...

image

PS:

... we can do it convention-based and just look at a collection of configuration files in some location and parse them all.

I'm currently trying to avoid using any ambient config files ... because I assume this will cause big trouble if the server and scanner are running on different machines, because the config files will most likely be only stored on the server and not be accessible for the client. Am I missing some SonarQube API functionality that would allow to use config files instead of Sonar config properties ???

@drywolf
Copy link
Contributor Author

drywolf commented Jul 6, 2016

Small update to the used names ... @Pablissimo let me know if this would be ok for you:

image

- multiple separated rule config collections now supported
- added parsing & handling of technical debt properties for rules
- added some more test coverage for rule parsing & handling
@coveralls
Copy link

coveralls commented Jul 6, 2016

Coverage Status

Coverage decreased (-0.2%) to 77.726% when pulling 0a49301 on drywolf:master into dc44ea1 on Pablissimo:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.2%) to 77.726% when pulling 1235047 on drywolf:master into dc44ea1 on Pablissimo:master.

@drywolf
Copy link
Contributor Author

drywolf commented Jul 6, 2016

@Pablissimo please review the latest commit, which hopefully should now be fine to wrap up this PR.

The changes that I made (in respect to the above points):

  • I made the rules config property global-only
  • the property for defining the rule-configurations is now more advanced than before (multiple collections of rule configs can be added / changed / removed in the plugin settings)
  • the property for the rule-configurations now is named sonar.ts.ruleconfigs
  • the code should now be camelCase only
  • I added support for the technical-debt rule-properties

@Pablissimo
Copy link
Owner

Amazing thanks - will be Sunday before I get to it probably but I've got a good feeling!

@Pablissimo Pablissimo merged commit 1235047 into Pablissimo:master Jul 23, 2016
Pablissimo added a commit that referenced this pull request Jul 23, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants