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

Implemented skip violations #200

Merged
merged 1 commit into from
Dec 21, 2018
Merged

Conversation

dbalabka
Copy link
Contributor

@dbalabka dbalabka commented Dec 3, 2018

Initial implementation of #199

Scope

Implement possibility to skip class dependency violation in depfile.yml. There will be new configuration section skip_violations (maybe skipped_violations better?). The developer can configure it in the following way:

skip_violations:
  Library\LibClass:
    - Core\CoreClass
    - Core\AnotherClass

skip_violations section contains associative array where key (Library\LibClass) is the name of dependant class and values (Core\CoreClass, Core\AnotherClass) are dependency classes.

Future scope

There might be implemented possibility for PHP functions dependency as well. In this case, we can specify functions names instead of class names. PHP allows to create functions with names which can be equal to class names in the same namespace:

<?php

namespace Foo;

class bar {
    function __invoke()
    {
        echo __CLASS__ . PHP_EOL;
    }
}

function bar() {
    echo __FUNCTION__ . PHP_EOL;
}

bar();
(new bar())();

output:

Foo\bar
Foo\bar

So function names should be identified differently from clasess. We can sufix each function name with ():

skip_violations:
  Library\LibClass:
    - Foo\bar()
    - Foo\bar

TODO

  • Implement class violations skipping possibility
  • Unit tests
  • Update documentation

@timglabisch
Copy link
Collaborator

thanks for trying to make deptrac better,

i am unsure about this PR. I would agree that we need some way to skip violations but there are a bunch of issues with this approach.

when i wrote deptrac i had the idea that a dependency is something abstract. in fact we (mostly) use deptrac on the "class"-level but if we think further dependencies has nothing to do with classes.

for example we could write a npath collector that collects every function that is "too complex". than you could write a rule to prevent some parts of your code to prevent on too complex code. You would end up with a dependency between (may) 2 functions, not classes.
skip_violations is only build on class level.

atm. i am unsure about how to solve this issue but i guess the approach is not generic enough.

@dbalabka
Copy link
Contributor Author

dbalabka commented Dec 4, 2018

@timglabisch agree with you.
Talking about classes and functions. I can change the configuration format to support both functions and classes to avoid configuration BC breaks. Or it might be improved in future releases until \SensioLabs\Deptrac\DependencyResult\DependencyInterface does support only classes.

When you integrate DepTrac in CI/CD with large code base (our case) all dependencies cannot be easily fixed, because it would take a lot of developer-hour. IMO DepTrac definitely should support violation skip possibility otherwise integration process might not be so smooth.

@dbalabka
Copy link
Contributor Author

dbalabka commented Dec 4, 2018

@timglabisch as you can see in #199 we have implemented a workaround using existing functionality, but not the best option. We would like to see jUnit support as well to see and control the amount of existing skipped violations. Also, an approach described in #199 provides a possibility to skip some class completely from an analysis, but in the scope of this PR I have implemented a possibility to skip exact violation which minimizes probability to introduce new violations.

@dbalabka
Copy link
Contributor Author

@timglabisch any ideas? Should I continue work on PR or you will just close it?

@smoench
Copy link
Contributor

smoench commented Dec 11, 2018

I think this would be good start having a skipping mechanism. If there is somebody having the use case skipping functions/npath then they/we should start thinking about abstracting the mechanism.

@torinaki 👍 feel free to continue

@smoench smoench added this to the 0.4 milestone Dec 19, 2018
@smoench
Copy link
Contributor

smoench commented Dec 19, 2018

@torinaki Some documentation in the README.md would be great :)

README.md Outdated Show resolved Hide resolved
@dbalabka
Copy link
Contributor Author

@smoench , @timglabisch it seems that PR is ready for review.

src/Configuration/ConfigurationSkippedViolation.php Outdated Show resolved Hide resolved
src/Configuration/ConfigurationSkippedViolation.php Outdated Show resolved Hide resolved
src/RulesetEngine.php Outdated Show resolved Hide resolved
Copy link
Contributor

@DavidBadura DavidBadura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

except for the little things 👍

@smoench
Copy link
Contributor

smoench commented Dec 20, 2018

@torinaki Would be great if you could rebase on actual master. I extended the travis ci config to enforce static analysis. As soon as the pipeline gets green I will merge your PR :)

@dbalabka dbalabka force-pushed the skip-violations branch 3 times, most recently from 7a895db to 0f73927 Compare December 20, 2018 17:23
@smoench
Copy link
Contributor

smoench commented Dec 21, 2018

@torinaki It seems you have just a few coding style issues. They could be easily fixed with php-cs-fixer. Just download the latest phar-file and run following command php php-cs-fixer.phar fix. Thanks :)

@dbalabka
Copy link
Contributor Author

@smoench CS is fixed

@smoench smoench merged commit c615abe into qossmic:master Dec 21, 2018
@smoench
Copy link
Contributor

smoench commented Dec 21, 2018

Thank you @torinaki

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

Successfully merging this pull request may close these issues.

None yet

4 participants