RulesetEngine should only use ConfigurationRuleset

[patrickkusebauch]

A good first issue to pick up - it is a small refactor.

Right now it depends on Configuration and all the methods it calls are used solely in the RulesetEngine. Therefore all of those should be moved into ConfigurationRuleset.

[mchekin]

@patrickkusebauch
I would like to look into it.

[mchekin]

@patrickkusebauch

RulesetEngine calls both Configuration::getSkipViolations() and Configuration::ignoreUncoveredInternalClasses().

Does it mean we need to move the skipViolations and ignoreUncoveredInternalClasses fields into ConfigurationRuleset?

If so, does it also mean that we need to re-structure the skip_violations and ignore_uncovered_internal_classes configuration keys under the ruleset configuration key (which the source of data for ConfigurationRuleset)?

What do you think?

[patrickkusebauch]

RulesetEngine calls both Configuration::getSkipViolations() and Configuration::ignoreUncoveredInternalClasses().

Yes I know, this is what this issue is about. 😃

Does it mean we need to move the skipViolations and ignoreUncoveredInternalClasses fields into ConfigurationRuleset?

Yes.

If so, does it also mean that we need to re-structure the skip_violations and ignore_uncovered_internal_classes configuration keys under the ruleset configuration key (which the source of data for ConfigurationRuleset)?

This would be a breaking change, I would prefer if no breaking change happened to the depfile. So rather change how data for ConfigurationRuleset is gathered in the code.

What do you think?

I think going forward what you are proposing makes sense, but deciding on breaking changes is @dbrumann 's domain.

[mchekin]

@patrickkusebauch

I opened a pull-request: #628

Without any configuration structure changes.

[dbrumann]

tl;dr Go ahead with restructuring but try to add a BC-layer if possible.

I am planning on writing a BC policy at some point, that will hopefully make it easier to decide on how/what to refactor. My personal rule for now is that code changes are mostly fine, because people primarily use deptrac as phar file. What I consider a BC break is changes to the existing console command inputs/outputs and changes to existing depfile keys.

Since deptrac is still in 0.x we can break backwards compatibility between minor releases. For instance 0.14 changed the output of the debug commands. Since those are unlikely to be used in CI, I was fine with it. When it comes to the depfile I am a bit more hesitant, because BC-breaks in there will likely break any existing usage.

Moving the keys under ruleset absolutely makes sense to me. Maybe we can avoid a hard BC-break, by allowing both styles to live side by side and raising deprecation warnings for the old style. If not, we can still move forward but then I would like to see which other BC breaks are in that release and give people proper instructions on how to resolve them. See: https://symfony.com/doc/current/components/config/definition.html#deprecating-the-option